1610594 - In RDM device-picker menu, the overlayed checkbox/browser-logo looks pretty broken on Linux

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect, P2)

Description

[Daniel Holbert [:dholbert]]

Attachment: screenshot.png

STR:

1. Visit any page, e.g. this page here
2. Open responsive design mode (Ctrl+Shift+M on Linux)
3. Click the device switcher menu (which starts out labeled "Responsive" at top, to the left of center)
4. Pick any option, e.g. the top one "Galaxy S9"
5. Click the device switcher menu again.

ACTUAL RESULTS:

• Nearly all of the options have a Chrome or Safari browser logo shown inside of their checkbox
• When you pick one of these options, then it shows up with the checkbox and browser logo overlayed, which just looks like a jumble of unintelligible visual artifacts. (You can't really tell anymore what the logo is, or what the checkbox is.)

EXPECTED RESULTS:
No such overlaying. If we want to show the logo, we should ensure it remains visible & don't paint checkbox styling on top of it.

Note: this might be better/worse on different operating system themes, depending on the checkbox styling. It happens to look pretty terrible with the default theme for Ubuntu 19.10, which is what I'm using.

Comment 1

[Daniel Holbert [:dholbert]]

Also, incidentally, it looks like the browser logos are mispositioned/mis-sized (they run off the bottom of the checkbox a bit), but that's not as severe of an issue.

Comment 2

[Daniel Holbert [:dholbert]]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b02e985f21307a96117f9b1bddbe0c249401ad9b&tochange=b7864d9346d5fc85c4eaf38a973873195e1d56ec
--> regression from bug 1306198.

Also: I verified that current Firefox beta (73.0b7) is affected by this.

[Tracking Requested - why for this release]: Visual regression (broken-looking UI) introduced in the Firefox 73 nightly cycle.

Comment 3

[Daniel Holbert [:dholbert]]

daisuke, do you have cycles to look at this?

Per the screenshots on bug 1306198 (e.g. attachment 9113761 [details]), it looks like the intent was for the logos to show up next to the checkboxes, not on top of the checkboxes.

Comment 4

[Daniel Holbert [:dholbert]]

(And if it matters: I'm using Ubuntu 19.10, and the current Nightly 74.0a1 (2020-01-21) (64-bit) is affected.)

Comment 5

[Daisuke Akatsuka (:daisuke)]

Thank you for reporting, Daniel!
Yeah, I could confirm on Ubuntu 18 as well.
I'll take a look.

Comment 6

[Daisuke Akatsuka (:daisuke)]

I discussed it with Brian and Makoto.
So, if we need to show the checkbox and the icon on Ubuntu as well, it seems that we should modify the Linux widget as we are using the XUL element for the component.
However, instead, recently, DevTools started to use the menu items that are made by HTML in some places such as MeatballMenu.
https://searchfox.org/mozilla-central/source/devtools/client/framework/components/MeatballMenu.js
https://searchfox.org/mozilla-central/source/devtools/client/shared/components/menu/
So, I started to think to use the HTML component to fix this issue. Let me try the above approach.

Comment 7

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1610594: Use HTML menu component instead of xul menu. r?mtigley!,jdescottes!

Before now, we used XUL elements as the menu for device selection on RDM.
We introduced the icon on the XUL menuitem in bug 1306198, but the UI on Ubuntu
became weird looking. In order to fix this, we could think to modify Linux
gadget, but we replace to HTML menu components we are using in DevTools with
considering deXUL as well.

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1610594: Support icon for MenuButton component. r?jdescottes!

Depends on D61149

Comment 9

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1610594: Add a test for device selector button and the menu items. r?mtigley!

Depends on D61149

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ec353a434f9
Support icon for MenuButton component. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/c3f7182fd57f
Use HTML menu component instead of xul menu. r=mtigley,jdescottes
https://hg.mozilla.org/integration/autoland/rev/a9a2395b667f
Add a test for device selector button and the menu items. r=mtigley

Comment 11

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_device_selector_items.js && /browser_orientationchange_event.js

backout: https://hg.mozilla.org/integration/autoland/rev/b0f071e99da08c2a5b1fb406a422d8b71460e198

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a9a2395b667fd8d20da1881d0309c5c598fa3bb0&selectedJob=287679748

failure log:

• devtools/client/responsive/test/browser/browser_device_selector_items.js | The menu item is on the list -
  https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=287679748&repo=autoland&lineNumber=23958

• on subsequent push devtools/client/responsive/test/browser/browser_orientationchange_event.js | Orientation angle is 0 - 90 == 0 -
  https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=287679364&repo=autoland&lineNumber=36596

Comment 12

[Daisuke Akatsuka (:daisuke)]

Updated the patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dae8be5e3190ba592f8873504175189e3b436e29

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/393f197ad3fd
Support icon for MenuButton component. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8ebf947a89cc
Use HTML menu component instead of xul menu. r=mtigley,jdescottes
https://hg.mozilla.org/integration/autoland/rev/62d23c66eccf
Add a test for device selector button and the menu items. r=mtigley

Comment 14

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/393f197ad3fd
https://hg.mozilla.org/mozilla-central/rev/8ebf947a89cc
https://hg.mozilla.org/mozilla-central/rev/62d23c66eccf