Don't record impressions for hidden content within collapsed <details> elements

Bug: 412621009
Change-Id: I55a6656db1200a2a85c8d65879a023029d667de8
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6842296
Reviewed-by: Ergün Erdoğmuş <[email protected]>
Auto-Submit: Danil Somsikov <[email protected]>
Commit-Queue: Ergün Erdoğmuş <[email protected]>

Author: danilsomsikov

front_end/ui/visual_logging/LoggingDriver.test.ts

@@ -147,6 +147,36 @@ describe('LoggingDriver', () => {
     await assertImpressionRecordedDeferred();
   });
 
+  it('does not log impressions for content in closed details element but does when opened', async () => {
+    await VisualLoggingTesting.LoggingDriver.startLogging({processingThrottler: throttler});
+
+    const details = document.createElement('details');
+    details.style.width = '100px';
+    details.style.height = '100px';
+    details.innerHTML = '<div id="details-content" jslog="TreeItem" style="width: 100px; height: 100px;"></div>';
+    renderElementIntoDOM(details);
+
+    let [work] = await expectCalled(throttle);
+    await work();
+    // This will fail with the bug, as an impression will be recorded.
+    sinon.assert.notCalled(recordImpression);
+
+    throttle.resetHistory();
+    recordImpression.resetHistory();
+
+    details.open = true;
+    // Opening details will trigger mutation observer.
+    [work] = await expectCalled(throttle);
+    await work();
+    sinon.assert.calledOnce(recordImpression);
+    assert.sameDeepMembers(recordImpression.firstCall.firstArg.impressions, [{
+                             id: getVeId('#details-content'),
+                             type: 1,
+                             width: 100,
+                             height: 100,
+                           }]);
+  });
+
   it('logs impressions on mutation in additional document', async () => {
     const iframe = document.createElement('iframe');
     renderElementIntoDOM(iframe);

front_end/ui/visual_logging/LoggingDriver.ts

@@ -153,7 +153,8 @@ export async function process(): Promise<void> {
     if (!loggingState.impressionLogged) {
       const overlap = visibleOverlap(element, viewportRectFor(element));
       const visibleSelectOption = element.tagName === 'OPTION' && loggingState.parent?.selectOpen;
-      const visible = overlap && (!parent || loggingState.parent?.impressionLogged);
+      const visible = overlap && element.checkVisibility({checkVisibilityCSS: true}) &&
+          (!parent || loggingState.parent?.impressionLogged);
       if (visible || visibleSelectOption) {
         if (overlap) {
           loggingState.size = overlap;

test/e2e/helpers/visual-logging-helpers.ts

@@ -48,10 +48,7 @@ export function veImpression(ve: string, context?: string, children?: TestImpres
   return {impressions: [key, ...veImpressionsUnder(key, children || []).impressions]};
 }
 
-function veImpressionForTabHeader(panel: string, options?: {closable: boolean}) {
-  if (options?.closable) {
-    return veImpression('PanelTabHeader', panel, [veImpression('Close')]);
-  }
+function veImpressionForTabHeader(panel: string) {
   return veImpression('PanelTabHeader', panel);
 }
 
@@ -60,15 +57,18 @@ export function veImpressionForMainToolbar(options?: {
   expectClosedPanels?: string[],
   dockable?: boolean,
 }) {
-  const regularPanels = ['elements', 'console', 'sources', 'network'];
+  const panels = [
+    'elements',
+    'console',
+    'sources',
+    'network',
+  ];
   if (!options?.dockable) {
-    regularPanels.push('timeline', 'heap-profiler', 'resources', 'lighthouse');
+    panels.push('security', 'chrome-recorder', 'timeline', 'heap-profiler', 'resources', 'lighthouse');
   }
 
-  const closablePanels =
-      options?.dockable ? [] : ['security', 'chrome-recorder'].filter(p => !options?.expectClosedPanels?.includes(p));
-  if (options?.selectedPanel && !regularPanels.includes(options?.selectedPanel)) {
-    closablePanels.push(options.selectedPanel);
+  if (options?.selectedPanel && !panels.includes(options?.selectedPanel)) {
+    panels.push(options.selectedPanel);
   }
 
   const dockableItems = options?.dockable ?
@@ -80,8 +80,7 @@ export function veImpressionForMainToolbar(options?: {
       [];
 
   return veImpression('Toolbar', 'main', [
-    ...regularPanels.map(panel => veImpressionForTabHeader(panel)),
-    ...closablePanels.map(panel => veImpressionForTabHeader(panel, {closable: true})),
+    ...panels.map(panel => veImpressionForTabHeader(panel)),
     veImpression('Toggle', 'elements.toggle-element-search'),
     veImpression('Action', 'settings.show'),
     veImpression('DropDown', 'main-menu'),
@@ -108,12 +107,10 @@ export function veImpressionForElementsPanel(options?: {dockable?: boolean}) {
     veImpression('Pane', 'styles', [
       veImpression('Section', 'style-properties', [veImpression('CSSRuleHeader', 'selector')]),
       veImpression('Section', 'style-properties', [
-        veImpression('Action', 'elements.new-style-rule'),
         veImpression('CSSRuleHeader', 'selector'),
         veImpression('Tree', undefined, [
-          veImpression('TreeItem', 'display', [veImpression('Toggle'), veImpression('Key'), veImpression('Value')]),
+          veImpression('TreeItem', 'display', [/* veImpression('Toggle'), */veImpression('Key'), veImpression('Value')]),
           veImpression('TreeItem', 'margin', [
-            veImpression('Toggle'),
             veImpression('Key'),
             veImpression('Expand'),
             veImpression('Value'),
@@ -134,10 +131,10 @@ export function veImpressionForElementsPanel(options?: {dockable?: boolean}) {
 export function veImpressionForDrawerToolbar(options?: {
   selectedPanel?: string,
 }) {
-  const closeablePanels = options?.selectedPanel ? [options?.selectedPanel] : [];
+  const panels = options?.selectedPanel ? [options?.selectedPanel] : [];
   return veImpression('Toolbar', 'drawer', [
     veImpressionForTabHeader('console'),
-    ...closeablePanels.map(panel => veImpressionForTabHeader(panel, {closable: true})),
+    ...panels.map(panel => veImpressionForTabHeader(panel)),
     veImpression('DropDown', 'more-tabs'),
     veImpression('Close'),
   ]);