1800417 - Fix the use of both `toolbarbutton-1` and `subviewbutton` CSS classes in extension widgets

Status: RESOLVED FIXED

(WebExtensions :: Frontend, task, P3)

Description

[William Durand [:willdurand]]

In Bug 1798324, we updated the style of the extension (CUI) widgets to match the Unified Extensions UI. These widgets are "custom" widgets and have two buttons. One of them is the primary (or action) button, also known as "view button" for the widgets of type "custom".

The view button in a custom widget is placed in the toolbar when the widget is pinned. We use the .toolbarbutton-1 class on this button for that reason. In addition, we use .subviewbutton on this same button when the widget is placed in a panel (unified extensions panel in most cases). See: https://searchfox.org/mozilla-central/rev/219df29d0fb5d8928ae41bba4a605046de411cf0/browser/components/extensions/parent/ext-browserAction.js#229-233

We should not use these two CSS classes on the same button. There were a few ideas in https://phabricator.services.mozilla.com/D161036.

Comment 1

[William Durand [:willdurand]]

Attachment: Bug 1800417 - Fix the use of both `toolbarbutton-1` and `subviewbutton` CSS classes in extension widgets. r?mconley!,Itiel

Comment 2

[William Durand [:willdurand]]

Attachment: screenshot of a problem with the use of .toolbarbutton-1

Fixing this bug would also fix a minor difference between CUI widgets and custom elements in the panel: when -moz-window-inactive is set, toolbar buttons have an opacity of 0.5, which essentially makes them less visible. That is not the case for the custom elements (at the bottom of the list) as seen in the screenshot.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab00cf793f29
Fix the use of both `toolbarbutton-1` and `subviewbutton` CSS classes in extension widgets. r=Itiel,mconley,dao,rpl

Comment 4

[Cristina Horotan [:chorotan]]

Backed out changeset ab00cf793f29 (bug 1800417) for causing failures at browser_unified_extensions_overflowable_toolbar.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/4c85944d11f569aea754ebbec96cdce3a3b4b652

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=IlKcmMFBSeOjGcarfI9nkw.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=ab00cf793f293710cbd216cd5c0c62fed1d6040e

Failure log: https://treeherder.mozilla.org/logviewer?job_id=403965999&repo=autoland&lineNumber=38794

Comment 5

[William Durand [:willdurand]]

I am still trying to fix Bug 1798683 to unlock the patch for this bug..

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6dbcf3dd30c4
Fix the use of both `toolbarbutton-1` and `subviewbutton` CSS classes in extension widgets. r=Itiel,mconley,dao,rpl

Comment 7

[Sandor Molnar[:smolnar]]

Meant to back out bug 1798683
Will reland this.
Sorry

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/549ce0549c2e
Fix the use of both `toolbarbutton-1` and `subviewbutton` CSS classes in extension widgets. r=Itiel,mconley,dao,rpl

Comment 9

[Sandor Molnar[:smolnar]]

This is the actual culprit. Backfills confirm it
Backed out for causing bc failures in browser/components/extensions/test/browser/browser_unified_extensions_overflowable_toolbar.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/883042d84c6a74e474c0fcb4be8cb93fe3909b77

Push with failures

Failure log

Comment 10

[William Durand [:willdurand]]

sigh, I'll take yet another look..

Comment 11

[William Durand [:willdurand]]

Attachment: mozilla-test-fail-screenshot_58ssxu5c.png

:mconley I am not able to land this patch because of an obscure bug on Linux, that I cannot reproduce myself. I can consistently reproduce on Try, though and the screenshot (attached here) reminds me of Bug 1798973, which we couldn't reproduce either. Could you please take a look?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Here's a try push for that test directory on the same platform with some added profiler markers and profiling enabled. Let's see if we can deduce what's happening here: https://treeherder.mozilla.org/jobs?repo=try&revision=94d9461452a2703e07a8af18bf8aa0a4159dcc0a

Will leave needinfo for now while I wait for the build to complete.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

So the issue seems to be that the OverflowableToolbar reports itself as not being enabled at the time that the resize event occurs:

https://share.firefox.dev/40H2Iy3

Looking into why that might be...

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Another try push with more logging to see if we can find where the OverflowableToolbar is being disabled: https://treeherder.mozilla.org/jobs?repo=try&revision=4b7cb913a8d350f7fd44289399d1a08bfb0dbd5b

Comment 15

[Mike Conley (:mconley) (:⚙️)]

The problem was that a previous test (browser_unified_extensions_cui.js) had fired an event on the toolbox that disabled the OverflowableToolbar for all windows to simulate entering customize mode, and then didn't fire the event to simulate exiting it again.

This seems to fix the test for me: https://hg.mozilla.org/try/rev/962d9ce798e71a42203d8376ccc5b1bd59344680

Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=a4b09b4711717be1d53b06fb5766d329adcbc53f

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d26e7747cf69
Fix the use of both `toolbarbutton-1` and `subviewbutton` CSS classes in extension widgets. r=Itiel,mconley,dao,rpl

Comment 17

[Marian-Vasile Laza]

Backed out changeset d26e7747cf69 (Bug 1800417) for bc failures on browser_unified_extensions_overflowable_toolbar.js.
Backout link
Push with failures <--> bc7
Failure Log

Comment 18

[William Durand [:willdurand]]

Attachment: Bug 1800417 - Make sure we exit customization mode in browser_unified_extensions_cui.js. r?rpl!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d88c5a272101
Make sure we exit customization mode in browser_unified_extensions_cui.js. r=rpl

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e391918bb558
Fix the use of both `toolbarbutton-1` and `subviewbutton` CSS classes in extension widgets. r=Itiel,mconley,dao,rpl

Comment 21

[Cristian Tuns]

Backed out for causing mochitest failures in browser_unified_extensions_overflowable_toolbar.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_unified_extensions_overflowable_toolbar.js | Test timed out -

Comment 22

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/d88c5a272101

Comment 23

[William Durand [:willdurand]]

Attachment: Bug 1800417 - Add a test case with two windows. r?rpl!

Depends on D162712

Depends on D162712

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a306b471b9fb
Fix the use of both `toolbarbutton-1` and `subviewbutton` CSS classes in extension widgets. r=Itiel,mconley,dao,rpl

Comment 25

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/a306b471b9fb

Comment 26

[Justin Peter]

This bug caused the default extension icon (i.e. for an extension without an icon) to change from white to black in dark mode.

I can provide more details or file a bug if needed, but I don't have time to at this exact moment; I just mozregressioned this and thought I'd leave a note.

Comment 27

[Itiel]

(In reply to Justin Peter from comment #26)

This bug caused the default extension icon (i.e. for an extension without an icon) to change from white to black in dark mode.

I can provide more details or file a bug if needed, but I don't have time to at this exact moment; I just mozregressioned this and thought I'd leave a note.

Thanks, should be fixed in bug 1817865.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30c8fc980ba2
Add a test case with two windows. r=rpl

Comment 29

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/30c8fc980ba2