Use :root instead of #main-window in browser/base/content/browser.css

RESOLVED FIXED in Firefox 65

Status

()

task
P3
normal
RESOLVED FIXED
3 years ago
6 days ago

People

(Reporter: dao, Assigned: kaczmarek_adrian, Mentored)

Tracking

Trunk
Firefox 65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [lang=css])

Attachments

(1 attachment, 1 obsolete attachment)

47 bytes, text/x-phabricator-request
Details | Review
(Reporter)

Description

3 years ago
:root is potentially cheaper than #main-window, because IDs aren't actually guaranteed to be unique, so ID selectors are treated just like class selectors under the hood.

#main-window occurrences:

https://dxr.mozilla.org/mozilla-central/search?q=%23main-window+file%3A.css+-path%3Aobj&redirect=false
(Reporter)

Comment 2

3 years ago
Comment on attachment 8778617 [details]
Bug 1292878 - Use :root instead of #main-window;

I've pushed this to the try server to take some screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8&newProject=try&newRev=7dc7db5a144bdff04277dd78f9b58f956c1c8fa6

This seems to cause some problems on Windows 7 and 8 presumably because the specificity of the selectors changes. If you leave out the browser-aero.css changes for now, we can land the rest and I can look into fixing browser-aero.css separately.
Attachment #8778617 - Flags: review?(dao+bmo)
(Reporter)

Updated

3 years ago
Assignee: nobody → sumi29

Comment 3

3 years ago
I half-anticipated that these changes would cause problems on Windows, but I lack access to a Windows machine so I couldn't really confirm. I'll revert the changes to browser-aero.css.

Comment 4

3 years ago
Comment on attachment 8778617 [details]
Bug 1292878 - Use :root instead of #main-window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69856/diff/1-2/
Attachment #8778617 - Flags: review?(dao+bmo)

Comment 6

3 years ago
(In reply to Dão Gottwald [:dao] from comment #5)
> This is better, but there are still some regressions on Windows 7 :/
> 
> https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> central&oldRev=6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8&newProject=try&newRev
> =68cd3149f38297c41f70e6186aec80b86e02de67

How would we narrow down what CSS is causing the regressions? It could be anything under the shared folder, or the Windows specific files. The fact that only Win7 is regressing is kinda weird though.
(Reporter)

Comment 7

3 years ago
(In reply to Sumit Tiwari from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > This is better, but there are still some regressions on Windows 7 :/
> > 
> > https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> > central&oldRev=6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8&newProject=try&newRev
> > =68cd3149f38297c41f70e6186aec80b86e02de67
> 
> How would we narrow down what CSS is causing the regressions? It could be
> anything under the shared folder, or the Windows specific files. The fact
> that only Win7 is regressing is kinda weird though.

I'm afraid the only way is reviewing the patch manually and making educated guesses, or building locally on Windows and reverting the changes one by one and see what happens :(
(Reporter)

Comment 8

3 years ago
(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to Sumit Tiwari from comment #6)
> > (In reply to Dão Gottwald [:dao] from comment #5)
> > > This is better, but there are still some regressions on Windows 7 :/
> > > 
> > > https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> > > central&oldRev=6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8&newProject=try&newRev
> > > =68cd3149f38297c41f70e6186aec80b86e02de67
> > 
> > How would we narrow down what CSS is causing the regressions? It could be
> > anything under the shared folder, or the Windows specific files. The fact
> > that only Win7 is regressing is kinda weird though.
> 
> I'm afraid the only way is reviewing the patch manually and making educated
> guesses, or building locally on Windows and reverting the changes one by one
> and see what happens :(

I could also push more patches for you to the try server and get screenshots. Getting screenshots takes some time, but we're not in a rush. I could even push a bunch of different experimental patches in parallel.
(Reporter)

Updated

3 years ago
Attachment #8778617 - Flags: review?(dao+bmo)

Comment 9

3 years ago
I'll hopefully get access to a Windows machine soon enough, so I can start experimenting. It would still be helpful to have screenshots though, so that I don't miss out on something.
(Reporter)

Updated

3 years ago
Priority: -- → P3

Comment 10

11 months ago
Is it still actual? I'm from WhatCanIDoForMozilla and BugAhoy.
Hi Paul! Sorry, this bug slipped past the radar - I'm needinfo'ing Dao, the mentor assigned to this bug, for you.
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 12

11 months ago
Yeah, this is still relevant. I've been trying to replace #main-window with :root when touching such rules and have pushed for this in reviews as well, so with some luck this bug is bit easier now than it was two years ago. Maybe not a good first bug, though.
Assignee: sumi29 → nobody
Flags: needinfo?(dao+bmo)
Whiteboard: [good first bug][lang=css] → [lang=css]
I'd like to take this. I'm not on Windows anymore, so I would need somebody to verify it(to not blame all Windows on Nightly users) if this could be still a problem for Windows. Otherwise, I can replace everything to `:root`? I assume I can use artifact builds here?
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 14

6 months ago
(In reply to Jens Hausdorf :jens1o [ni? me] from comment #13)
> I'd like to take this. I'm not on Windows anymore, so I would need somebody
> to verify it(to not blame all Windows on Nightly users) if this could be
> still a problem for Windows.

We can run the browser-screenshots-e10s suite on the try server to test this on all three platforms.

> Otherwise, I can replace everything to `:root`?
> I assume I can use artifact builds here?

Yes and yes.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 15

5 months ago
Posted file [mq]: 1292878
Replaced #main-window with :root in the browser/base/content/browser.css
(Reporter)

Updated

5 months ago
Assignee: nobody → kaczmarek_adrian
Status: NEW → ASSIGNED
Summary: Use :root instead of #main-window → Use :root instead of #main-window in browser/base/content/browser.css
(Reporter)

Updated

5 months ago
Attachment #8778617 - Attachment is obsolete: true
Attachment #9029781 - Attachment is obsolete: true

Comment 16

5 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bacd54533618
Use :root instead of #main-window in browser/base/content/browser.css. r=dao
(Reporter)

Updated

5 months ago
Attachment #9029781 - Attachment is obsolete: false

Comment 17

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bacd54533618
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
(Reporter)

Updated

18 days ago
See Also: → 1541799
(Reporter)

Updated

16 days ago
Type: defect → task
(Reporter)

Updated

16 days ago
See Also: → 1542187
(Reporter)

Updated

12 days ago
See Also: → 1543043
(Reporter)

Updated

6 days ago
See Also: → 1544680
You need to log in before you can comment on or make changes to this bug.