Closed Bug 1708370 Opened 3 years ago Closed 1 year ago

New wpt failures in /html/rendering/non-replaced-elements/ [form-controls/resets.html, lists/lists-styles.html]

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
116 Branch
Tracking Flags:
Tracking Status
firefox116 --- fixed

People

(Reporter: wpt-sync, Assigned: vhilla)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [wpt])

Attachments

(1 file)

Bug 1708370 - adjust UA stylesheet to match spec for form rendering. r=emilio
48 bytes, text/x-phabricator-request
Details | Review

Syncing wpt PR 28040 found new untriaged test failures in CI

Tests Affected

Firefox-only failures

/html/rendering/non-replaced-elements/form-controls/resets.html
<option> (in <select><optgroup>) - text-align: FAIL
<optgroup> - line-height: FAIL
<optgroup> - text-indent: FAIL
<option> - line-height: FAIL
<option> - text-indent: FAIL

New Tests That Don't Pass

/html/rendering/non-replaced-elements/lists/lists-styles.html
<ol type="none"> - list-style-type: FAIL (Chrome: PASS)
<li type="none"> - list-style-type: FAIL (Chrome: PASS)
<ol type="NONE"> - list-style-type: FAIL (Chrome: PASS)
<li type="NONE"> - list-style-type: FAIL (Chrome: PASS)
<ol type="disc"> - list-style-type: FAIL (Chrome: PASS)
<ol type="DISC"> - list-style-type: FAIL (Chrome: PASS)
<ol type="circle"> - list-style-type: FAIL (Chrome: PASS)
<ol type="CIRCLE"> - list-style-type: FAIL (Chrome: PASS)
<ol type="square"> - list-style-type: FAIL (Chrome: PASS)
<ol type="SQUARE"> - list-style-type: FAIL (Chrome: PASS)

CI Results

Gecko CI (Treeherder)
GitHub PR Head

Notes

These updates will be on mozilla-central once bug 1708171 lands.

Note: this bug is for tracking fixing the issues and is not
owned by the wpt sync bot.

This bug is linked to the relevant tests by an annotation in
https://github.com/web-platform-tests/wpt-metadata. These annotations
can be edited using the wpt interop dashboard
https://jgraham.github.io/wptdash/

If this bug is split into multiple bugs, please also update the
annotations, otherwise we are unable to track which wpt issues are
already triaged. Resolving as duplicate or closing this issue should
be cause the bot to automatically update or remove the annotation.

(In reply to Web Platform Test Sync Bot (Matrix: #interop:mozilla.org) from comment #0)

Syncing wpt PR 28040 found new untriaged test failures in CI

Tests Affected

Firefox-only failures

/html/rendering/non-replaced-elements/form-controls/resets.html
<option> (in <select><optgroup>) - text-align: FAIL
<optgroup> - line-height: FAIL
<optgroup> - text-indent: FAIL
<option> - line-height: FAIL
<option> - text-indent: FAIL

The above still fail ^^^

New Tests That Don't Pass

/html/rendering/non-replaced-elements/lists/lists-styles.html

These two sub-tests from the original report still fail in Fx 112:
<li type="none"> - list-style-type: FAIL (Chrome: PASS)
<li type="NONE"> - list-style-type: FAIL (Chrome: PASS)

Others below were fixed:

<ol type="none"> - list-style-type: FAIL (Chrome: PASS)

Fixed

<ol type="NONE"> - list-style-type: FAIL (Chrome: PASS)

Fixed

<ol type="disc"> - list-style-type: FAIL (Chrome: PASS)

Fixed

<ol type="DISC"> - list-style-type: FAIL (Chrome: PASS)

Fixed

<ol type="circle"> - list-style-type: FAIL (Chrome: PASS)

fixed

<ol type="CIRCLE"> - list-style-type: FAIL (Chrome: PASS)

fixed

<ol type="square"> - list-style-type: FAIL (Chrome: PASS)

fixed

<ol type="SQUARE"> - list-style-type: FAIL (Chrome: PASS)

fixed

These interop-2023-forms tests still fail:
<option> (in <select><optgroup>) - text-align: FAIL
<optgroup> - line-height: FAIL
<optgroup> - text-indent: FAIL
<option> - line-height: FAIL
<option> - text-indent: FAIL

Hi Emilio, I recalled when we talked about these tests a while ago on Matrix, the effort here would either fixing the buggy tests or some tweak in forms.css. I plan to assign this to our team's student worker, Vincent. If you have more pointers for him to start from, it's highly appreciated. :)

Flags: needinfo?(emilio)

So it seems some of the styles in here are not per spec.

It seems line-height comes from bug 82265, text-indent comes from bug 56253. We should check whether we can remove them (specially after bug 1744009 I think we probably can), but we should test <select> and <select multiple> with various style combinations to confirm rendering matches other browsers.

Flags: needinfo?(emilio)

Thanks for the pointers. After correcting the styles in forms.css, optgroup is still failing for line-height, as it's not inheriting the style. line-height is an inherited property though and works correctly on other elements.

<div id=container style="line-height: 5px">
  <optgroup id=ogt></optgroup>
</div>
<script>
console.log(getComputedStyle(ogt).getPropertyValue("line-height"));
</script>

This writes normal to console.

:emilio, do you know what could cause this?

Flags: needinfo?(emilio)

The font shorthand also resets line-height per spec, so it's being reset by font: -moz-list. You can see this easily if you enable "Show browser styles" in the devtools settings fwiw :)

Flags: needinfo?(emilio)

Alright, so far it looks good :) Two more things.

Bug 82265, Bug 56253 and Bug 645642 added line-height, text-indent and text-align for more than the elements relevant to this bug. Should I look into removing these rules from other elements too, if it is not part of the HTML spec?

And looking at this test from Bug 82265, the optgroup is rendered differently in Firefox vs Blink, WebKit, when no label is provided. The reason is that Firefox uses a ::before for the label that has zero height for an empty text. The specification is ambiguous on this. Should I

  • file a spec issue for that
  • file a bug with Firefox for using a ::before / having zero height?
Flags: needinfo?(emilio)

(In reply to Vincent Hilla [:vhilla] from comment #6)

Bug 82265, Bug 56253 and Bug 645642 added line-height, text-indent and text-align for more than the elements relevant to this bug. Should I look into removing these rules from other elements too, if it is not part of the HTML spec?

Possibly, but I'd do it in separate bugs or at least separate patches to be able to back out easily if there are issues.

And looking at this test from Bug 82265, the optgroup is rendered differently in Firefox vs Blink, WebKit, when no label is provided. The reason is that Firefox uses a ::before for the label that has zero height for an empty text. The specification is ambiguous on this. Should I

  • file a spec issue for that
  • file a bug with Firefox for using a ::before / having zero height?

If you change the optgroup:before rule to have content: "\200b" attr(label), does it work then?

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

If you change the optgroup:before rule to have content: "\200b" attr(label), does it work then?

Sure does. It does seem wrong to me though to use ::before for this.

Assignee: nobody → vhilla
Status: NEW → ASSIGNED

(In reply to Vincent Hilla [:vhilla] from comment #8)

Sure does. It does seem wrong to me though to use ::before for this.

It is not great to use generated content for this indeed, but I don't think changing it should be in scope for this bug.

Separate bug is fine, but also unless it actively creates compat issues or unless the spec defines it properly in a way that it's interoperable, I don't think changing if just for the sake of changing it is worth it.

Severity: -- → S3
Priority: -- → P2

optgroup::before is tracked in Bug 1498358

Pushed by vhilla@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24043b03134b
adjust UA stylesheet to match spec for form rendering. r=emilio

:emilio (In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

If you change the optgroup:before rule to have content: "\200b" attr(label), does it work then?

this breaks test_HTMLSpec.html and browser_selectables.js, as another staticText is inserted within the accessibility information.

I can update the expectation data, but do you think an additional empty staticText before any grouplabel is an accessibility problem? If so, we could also accept for now that empty label means the label will have zero height.

Flags: needinfo?(vhilla)
Flags: needinfo?(emilio)

301 Jamie, but my guess is that the extra zwsp is harmless.

Flags: needinfo?(emilio) → needinfo?(jteh)

It's probably harmless, but ugly/slightly confusing, since it's visible to screen reader review cursors and inspection tools. Most users probably won't see it, but some might. I think it's acceptable for now if that's the cleanest solution.

Flags: needinfo?(jteh)
Pushed by vhilla@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5a58f2c2915
adjust UA stylesheet to match spec for form rendering. r=emilio

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox116: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: