Set [orient], [pack], [dir], and [align] styles with CSS instead of XUL layout JS-properties
Categories
(Core :: Layout, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox72 | --- | fixed |
People
(Reporter: bgrins, Assigned: bgrins)
References
(Blocks 3 open bugs)
Details
Attachments
(2 files)
This is extracted out of a conversation at https://mozilla.logbot.info/content/20191029#c16723368-c16723416 and discussion with dholbert.
Here's what I think we need to do to pave the way for supporting -moz-box HTML elements (while we in parallel work on xul flex emulation). This would also hopefully help with bugs like the orient property sometimes being ignored (Bug 1590513).
- Move the following rules out of of the preference specific media query, and add
!important
so they better reflect the current semantics of xul flex properties https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/toolkit/content/xul.css#585-598. - Change callers of xulElement.align = "start" to xulElement.setAttribute("align", "start") - I'm not sure if this is strictly necessary since I think the property change will ultimately change the attribute anyway
- Change XUL layout code that was looking up the align property on the node to instead read the computed style (which would be set from the attribute + the rule in xul.css), or read the attribute directly
Also, we should probably change those attributes to be CSS classes instead since they are faster. Will require rewriting frontend code which could happen before or the above changes.
This doesn't address handling [flex], [oridinal], [width], and [height] which would either need attr in CSS or more explicit setting of styles in the frontend / layout code. We could tackle that later, if the simpler cases seem to work.
Assignee | ||
Comment 1•5 years ago
|
||
Neil, does this sound like it would work? It's sort of the opposite of Bug 1493962 in some ways, but I think it would similarly unblock xul flexbox emulation (at least for these properties in particular) and it will better support HTML elements
Comment 2•5 years ago
|
||
(To clarify, since this confused me at first in slack discussions -- the mentions of "property" in comment 0 are talking about JS properties, not CSS properties. i.e. xulElement.align = ...
. I would guess these JS-properties are effectively just aliases for the corresponding XUL attribute (and they currently have their layout impact via us reacting to the attribute-change), but I'm not sure.)
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
I believe if we made the proposed change we could drop things like https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/layout/xul/nsBoxFrame.cpp#396-406 since the "Check the style system first" block at https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/layout/xul/nsBoxFrame.cpp#388-394 should return the right thing based on the attribute.
Assignee | ||
Comment 4•5 years ago
|
||
We'd have to audit the in-content xul elements (minimal-xul.css to make sure we explicitly set box properties instead of relying on these attrs, since we don't load xul.css in the content document.
Assignee | ||
Comment 5•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #4)
We'd have to audit the in-content xul elements (minimal-xul.css to make sure we explicitly set box properties instead of relying on these attrs, since we don't load xul.css in the content document.
Stuff like this in particular: https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/layout/xul/nsScrollbarFrame.cpp#369-388. Emilio mentioned that doing this via classes would be faster, and optimizing scrollbars could be nice since they are so common.
Assignee | ||
Comment 6•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #5)
(In reply to Brian Grinstead [:bgrins] from comment #4)
We'd have to audit the in-content xul elements (minimal-xul.css to make sure we explicitly set box properties instead of relying on these attrs, since we don't load xul.css in the content document.
Stuff like this in particular: https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/layout/xul/nsScrollbarFrame.cpp#369-388. Emilio mentioned that doing this via classes would be faster, and optimizing scrollbars could be nice since they are so common.
The literal values here (flex, align, pack) should be able to be added directly to minimal-xul.css on the relevant tags, I would think.
Assignee | ||
Comment 7•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
Try is surprisingly green with that change (looks like one reftest failure): https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d058eda52611f975504d65266ae0c7ce26b9c20&selectedJob=273563966, and talos is quiet: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=541daa67b3bbc5776fa1fb378bd4b223c19e041c&newProject=try&newRevision=12a29d8b9c8a4d226eb458ec12192d419395465f&framework=1&showOnlyImportant=1.
I'm not sure if I missed other places that rely on xul elements + attributes, but it does seem to work so far (and probably unblocks issues like the workaround we did for Bug 15905130)
Assignee | ||
Comment 9•5 years ago
|
||
I'm guessing the reftest failure is because the test uses align="left" which our xul.css selectors don't match: https://searchfox.org/mozilla-central/source/layout/reftests/xul/text-crop.xul#3. Neil, should we rewrite [align="left"] to [align="start"] and [align="right"] to [align="end"] on XUL elements in the frontend? I see comments about them being deprecated in https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/layout/xul/nsBoxFrame.cpp#237-246.
Assignee | ||
Comment 10•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
Magnus, you'll want to rewrite any consumers of the deprecated "left" and "right" [align] values to "start" and "end", as done in https://phabricator.services.mozilla.com/D51164.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 13•5 years ago
|
||
Will rebase around Bug 1596296 once that lands
Comment 14•5 years ago
|
||
Comment 15•5 years ago
|
||
Comment 16•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c59883012b35
https://hg.mozilla.org/mozilla-central/rev/c58aa1b00988
Description
•