2008335 - Some preliminary changes before making vertical-align a shorthand.

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Comment 1

[Sajid Anwar [:sajidanwar]]

Attachment: Bug 2008335 - Update WPT to move bottom/center/top from alignment-baseline to baseline-shift per specification. r=#layout-reviewers

These values were moved to baseline-shift in https://github.com/w3c/csswg-drafts/issues/5180.

Comment 2

[Sajid Anwar [:sajidanwar]]

Attachment: Bug 2008335 - Decouple table cell alignment enum from CSS vertical-align enum. r=#layout-reviewers

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/98b169cc6a98
https://hg.mozilla.org/integration/autoland/rev/93184ae4a914
Update WPT to move bottom/center/top from alignment-baseline to baseline-shift per specification. r=layout-reviewers,emilio

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b55def863513
https://hg.mozilla.org/integration/autoland/rev/4d90f539da31
Decouple table cell alignment enum from CSS vertical-align enum. r=layout-reviewers,emilio

Comment 5

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/56957 for changes under testing/web-platform/tests

Comment 6

[Sajid Anwar [:sajidanwar]]

Attachment: Bug 2008335 - Fix boundary shaping WPT to properly test non-initial baseline-shift values. r=#layout-reviewers

The CSS Text specification says that text shaping should be broken if
the vertical-align is not baseline. This WPT treated a 0 value as
separate from baseline, but the CSS 2 specification makes clear that a
value of 0 is equivalent to baseline, so text shaping shouldn't be
broken in this case.

Furthermore, the CSS Inline Layout Model Level 3 makes it clear that not
breaking is the correct behavior. With the introduction of the shorthand
syntax, vertical-align: 0 is equivalent to the initial value of
baseline, since 0 is the initial value of baseline-shift. So I've
modified this test to use a non-initial baseline-shift length to test
the shaping behavior.

Related clarification issue filed to the CSS Working Group:
https://github.com/w3c/csswg-drafts/issues/13277

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d88d4c8d3d71
https://hg.mozilla.org/integration/autoland/rev/346397b33bb9
Fix boundary shaping WPT to properly test non-initial baseline-shift values. r=layout-reviewers,emilio,jfkthame

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/93184ae4a914
https://hg.mozilla.org/mozilla-central/rev/4d90f539da31
https://hg.mozilla.org/mozilla-central/rev/346397b33bb9

Comment 9

[Sajid Anwar [:sajidanwar]]

Attachment: Bug 2008335 - Refactor HTML align attribute values and CSS property mappings. r=#layout-reviewers

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8b60f09bd9bd
https://hg.mozilla.org/integration/autoland/rev/ebe58a418366
Refactor HTML align attribute values and CSS property mappings. r=layout-reviewers,emilio

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ade4bbd47bee
https://hg.mozilla.org/integration/autoland/rev/ea325717f026
Revert "Bug 2008335 - Refactor HTML align attribute values and CSS property mappings. r=layout-reviewers,emilio" for causing wpt failures.

Comment 13

[Goloman Adrian[:agoloman]]

Backed out for causing wpt failures.

• Backout link
• Push with failures
• Failure Log
• Failure Log @system-color-support.html

Comment 14

[Sajid Anwar [:sajidanwar]]

Found the cause of the issue — in nsGenericHTMLElement.cpp, the parse table for <table> element's align attribute includes "justify" as a valid value, but then doesn't do anything with it in HTMLTableElement.cpp when mapping it to CSS properties. This matches the HTML 4 specification which only allows "left"/"right"/"center".

When I refactored this out in my change, I kept basically the same structure as before where I parsed "justify" but only handled "left"/"right"/"center" in a switch. Except I added the unreachable assert to the default branch, which caused it to crash on "justify". Since this value is invalid and unused today, I'll remove it from the parse table.

---

No idea about the system-color-support.html failure. Suspect it must be intermittent or something, I can't repro and my change has nothing to do with colors.

Comment 15

[Sajid Anwar [:sajidanwar]]

Published a new revision of the patch to fix the failing WPTs. Here's a new try for this change which confirms that the WPTs are no longer failing: https://treeherder.mozilla.org/jobs?repo=try&revision=801710ab6ec4a463d16facda9b9027f5258329c5

The system-color-support.html failure is still here, but it definitely appears unrelated. I can see the same failure in a completely separate autoland push after the revert of my change already went through.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b406527cf117
https://hg.mozilla.org/integration/autoland/rev/e6019ace3b70
Refactor HTML align attribute values and CSS property mappings. r=layout-reviewers,emilio

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/e6019ace3b70