1625720 - Scope popup.css to menupopup and panel custom elements

Status: NEW

(Toolkit :: Themes, task, P3)

Description

[Tim Nguyen :ntim]

This is actually surprisingly straightforward, given that both these CEs already use shadow DOM and most (if not all) external styling already uses shadow parts.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1625720 - Scope popup.css to menupopup and panel custom elements.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64f788053239
Scope popup.css to menupopup and panel custom elements. r=dao

Comment 3

[Dorel Luca [:dluca]]

Backed out changeset 64f788053239 (bug 1625720) for mochitest failures in tests/chrome/test_panel.xhtml. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298846268&repo=autoland&lineNumber=11123

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=298847809&revision=64f788053239b1c3faa83e9f2340b1b1f82c6851

Backout:
https://hg.mozilla.org/integration/autoland/rev/023ec9bf4bd6e760d937071dbf8e716cf7219caf

Comment 4

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ba2c5ef33fda3d1e465f5642a36425922e17315

Comment 5

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91713493297963d8828f7911770ab2fd6c7dba41

It's not ideal, but this patch fixes the failure: https://hg.mozilla.org/try/rev/2b02c8ee22a6a1fc204a46e01f81a8b866490c82

I can't really tell if the failure was caused by the styling being applied too late (due to being loaded by the shadow root), or whether it was caused by the sole presence of the shadow DOM stylesheet in the panel custom elements. I'm guessing it's the former.

Either way, only the :host CSS rule is relevant to panels. The rest is of the popup.css stylesheet is only relevant to menupopup.

Dão, would you be ok with landing the patch I've linked above (or something similar) ?

Comment 6

[Dão Gottwald [:dao]]

Is the failure something that would affect real world users, or can we fix the test? Which of the properties you moved to xul.css is responsible for the failure? Can we move only that property instead of all?

Comment 7

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #6)

Is the failure something that would affect real world users, or can we fix the test?

I honestly don't know.

According to the logs, it seems to crash around: https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/toolkit/content/tests/chrome/window_panel.xhtml#71-73
for the "noautohide panel with titlebar" test

Which of the properties you moved to xul.css is responsible for the failure? Can we move only that property instead of all?

This is a Windows-only failure so I'm guessing only moving over the Windows properties would work. The only thing I know is that the min-width property is not needed as comment 4 shows.

I don't have a Windows machine and my time is limited until the end of May, so I really don't have time to figure out which exact property is behind it.

Dão, would you be ok with only moving over the Windows properties except for min-width, just to fix the regressions ? I can file a follow up bug and needinfo Neil Deakin on it for the time being.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #7)

Which of the properties you moved to xul.css is responsible for the failure? Can we move only that property instead of all?

This is a Windows-only failure so I'm guessing only moving over the Windows properties would work. The only thing I know is that the min-width property is not needed as comment 4 shows.

Can you narrow it down further to one property? Shouldn't be a lot of effort with a series of parallel try pushes...

Comment 9

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b774cfe43c5ce982b837930860453eb0752d0a57
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78a32cf3e56de7f7400cd89b9033eb8c74698c18
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89a29001d256a73009123fd3d0d778ae4d670163

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9432b1bd3d98
Scope popup.css to menupopup and panel custom elements. r=dao

Comment 11

[Alexandru Michis [:malexandru]]

Backed out changeset 9432b1bd3d98 for causing crashes in LayerComposite.

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=26579c47e9a9c9af4e27e995a365d0d152237d3b&searchStr=windows%2C10%2Cx64%2Cquantumrender%2Cshippable%2Copt%2Cmochitests%2Ctest-windows10-64-shippable-qr%2Fopt-mochitest-browser-chrome-e10s-1%2Cm%28bc1%29&tochange=de714602be8edf8e239d4f89e8d13e59129b8132&selectedJob=299325949

Failure logs:

• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299325949&repo=autoland&lineNumber=4971
• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299325954&repo=autoland&lineNumber=14889

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c0b3fa9021b
Scope popup.css to menupopup and panel custom elements. r=dao

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2c0b3fa9021b

Comment 14

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #7)

Dão, would you be ok with only moving over the Windows properties except for min-width, just to fix the regressions ? I can file a follow up bug and needinfo Neil Deakin on it for the time being.

The ifdef should have been around the whole rule, and the comment should have referenced the bug you were going to file. Can you please do that?

Comment 15

[Tim Nguyen :ntim]

Hi Dão,

I'm currently busy with writing up my masters' thesis (related to conic gradients in Firefox), would you mind taking care of potential necessary follow ups for this bug ?

Cheers,
Tim

Comment 16

[Dão Gottwald [:dao]]

Gonna have to back this out to address the regressions. Patch in bug 1634865.