Closed Bug 1592369 Opened 4 months ago Closed 3 months ago

Set [orient], [pack], [dir], and [align] styles with CSS instead of XUL layout JS-properties

Categories

(Core :: Layout, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 4 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).

  1. 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.
  2. 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
  3. 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.

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

Flags: needinfo?(enndeakin)
See Also: → 1493962

(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.)

Summary: Set [orient], [pack], [dir], and [align] styles with CSS instead of XUL layout properties → Set [orient], [pack], [dir], and [align] styles with CSS instead of XUL layout JS-properties
See Also: → 1590513

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.

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.

(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.

(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.

Blocks: 1590576
Attachment #9105063 - Attachment description: Bug 1592369 - WIP - Set [orient], [pack], [dir], and [align] styles with CSS instead of XUL layout JS-properties → Bug 1592369 - WIP - Set [orient], [pack], [dir], and [align] styles with CSS instead of reading the XUL layout attributes

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)

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.

Attachment #9105063 - Attachment description: Bug 1592369 - WIP - Set [orient], [pack], [dir], and [align] styles with CSS instead of reading the XUL layout attributes → Bug 1592369 - Set [orient], [pack], [dir], and [align] styles with CSS instead of reading the XUL layout attributes
See Also: → 1593023
See Also: → 1593303

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.

Flags: needinfo?(mkmelin+mozilla)
See Also: → 1593848

Thanks Brian, fixing it in bug 1594331.

Flags: needinfo?(mkmelin+mozilla)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Flags: needinfo?(enndeakin)

Will rebase around Bug 1596296 once that lands

Depends on: 1596296
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c59883012b35
Rewrite [align="left"] to [align="start"] and [align="right"] to [align="end"] in XUL elements r=NeilDeakin
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c58aa1b00988
Set [orient], [pack], [dir], and [align] styles with CSS instead of reading the XUL layout attributes r=NeilDeakin
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Regressions: 1599283
Regressions: 1600281
Blocks: 1589089
You need to log in before you can comment on or make changes to this bug.