1951562 - WPT css/css-ui/compute-kind-widget-generated/grouped-kind-of-widget-fallback-border-top-width-001.html assumes that getComputedStyle for <progress> elements' borders would return the same answer regardless of 'appearance'

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, task)

Description

[Daniel Holbert [:dholbert]]

[Filing this bug for one additional issue that will cause grouped-kind-of-widget-fallback-border-top-width-001.html to keep failing even after we fix the most prominent reason for it failing which is bug 1938683.]

STR:

1. Load this WPT and its reference case:
   https://wpt.live/css/css-ui/compute-kind-widget-generated/grouped-kind-of-widget-fallback-border-top-width-001.html
   https://wpt.live/css/css-ui/compute-kind-widget-fallback-all-elements-ref.html
2. Zoom in to make things easier to see (optional but encouraged)
3. Look at the top edge of the <progress> element in the bottom right corner, and switch back and forth between the testcase vs. reference case.

EXPECTED RESULTS: The test should pass.
ACTUAL RESULTS: There's a light-gray top border in the reference case which is missing in the testcase.

Superficially, it's sorta to-be-expected that the border is missing, since this element explicitly has border-width: 0px 1px 1px; in the testcase, which sets border-width to 0px. But that explicit styling actually comes from inspecting our computed styles -- it's not hardcoded in the test -- and our default styling is here and supposedly gives us a consistent 1px border on all sides:
https://searchfox.org/mozilla-central/rev/13d864c116029fe5d067d589005f49c1c46f74b5/layout/style/res/forms.css#673,679-680

progress {
...
  /* Default style in case of there is appearance: none; */
  border: 1px solid ThreeDShadow;

So there's some mystery as to how we end up with 0px 1px 1px here...

Comment 1

[Daniel Holbert [:dholbert]]

Hmm, so the issue is that getComputedStyle is lying here when we're rendering with the themed widget (which we do at the very start, until after we inspect and set border-top).

Using this data URI testcase...

data:text/html,<progress id="elem" value=0.3>

...and inspecting the computed style, I see these results:

>> getComputedStyle(elem).borderTopWidth
"0px"
>> getComputedStyle(elem).borderRightWidth
"0px"
>> getComputedStyle(elem).borderBottomWidth
"0px"
>> getComputedStyle(elem).borderLeftWidth
"0px"
>> getComputedStyle(elem).borderWidth
"1px"
>> getComputedStyle(elem).border
"1px solid rgb(227, 227, 227)"

i.e. the longhand properties all return 0px while we're showing a themed widget, even though the shorthand properties (border-width and border) report the true computed style from forms.css.

The disagreement in my console.log output above seems buggy. The 0px answers there clearly disagree with the actual computed styles, but maybe they're inserted to more accurately report the state of the themed widget (which indeed does not have a border around it), but it causes trouble for the test's assumptions.

The test assumes that we can read the computed border-width style of a themed widget, and then stick that into the specified style, and that'll produce exactly the appearance:none-like rendering. But that's not actually what happens because the first longhand property that we read (for the top side) ends up with a different value (0 in this case) which does trigger an appearance:none rendering, but ends up being the wrong border-width from what we would have gotten if we went directly to appearance:none.

I'm not sure if this is something we should change/fix in Gecko or not, but I think we can at least fix the test to reduce its assumptions in this area by making these widgets initially display:none or appearance:none, and setting the various border/background properties while we're in that state, and then removing display:none (or appearance:none) after we've finished setting the border/background properties.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: modified testcase which sets `appearance:none` to avoid picking up wrong answers from themed widgets

Comment 3

[Daniel Holbert [:dholbert]]

See attached testcase for an example of how we might modify https://wpt.live/css/css-ui/compute-kind-widget-generated/grouped-kind-of-widget-fallback-border-top-width-001.html (or really the script that generates it) to work around this. Here's the relevant JS, for reference -- the two lines with el.style.appearance (and the accompanying comment) are the only changes:

const props = "border-top-width,border-right-width,border-bottom-width,border-left-width".split(",");
for (const el of elements) {
  // Make the element *temporarily* 'appearance:none' so that we're not fooled
  // by any used values of properties that are specific to the themed widget:
  el.style.appearance = "none";
  for (const prop of props) {
    el.style.setProperty(prop, getComputedStyle(el).getPropertyValue(prop));
  }
  el.style.appearance = "";
}

That modified testcase (in comment 2) gets us the right answer -- it ends up with border-width: 1px; in the progress element's inline style, which is what we want/expect and is sufficient for that part to match the reference case.

zcorpan/emilio, would that sort of change make sense to you? (It feels slightly like cheating to dip our toes into appearance:none territory while trying to see whether something renders like appearance:none, but I think it's sorta required here, given that getComputedStyle explicitly behaves differently and returns used values rather than comptued values depending on the widget's appearance/theming status.)

Comment 4

[Daniel Holbert [:dholbert]]

In other words, tl;dr:

• The test right now assumes that border-* subproperties will all round-trip successfully, but that's not entirely true until after we've made the widget unthemed, because we lie about computed styles for themed widgets -- we return used values from layout rather than the values from the style system.

• So the first sub-property that the test tries to set (border-top in this case) will fail to round-trip in the way that the test expects it to, and it'll result in a test-failure. But the fact that we set some value for that property does successfully make the widget un-themed and gets the rest of the subproperties to round-trip successfully. So that's why only that one sub-property ends up being wrong.

• We can avoid this problem by making the widget explicitly appearance:none up-front, just while we're reading/setting the values of the properties we're interested in, and then removing the appearance styling at the end so that we're not benefiting from it in the actual final rendering.

Does that make sense?

Comment 5

[Emilio Cobos Álvarez [:emilio]]

The test right now assumes that border-* subproperties will all round-trip successfully, but that's not entirely true until after we've made the widget unthemed, because we lie about computed styles for themed widgets -- we return used values from layout rather than the values from the style system.

This used to be needed but is a violation of the spec.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

https://treeherder.mozilla.org/jobs?repo=try&revision=c6c1dbe561204de36d9a410affed43a460f5ae2b

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1951562 - Make ProgressBar appearance honor UA sheet borders. r=dholbert

Much like button and most other widgets. This is the lower-risk fix for
these tests and it makes sense.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1951562 - Do not return used border-width values from getComputedStyle(). r=#style,#layout

border-*-width is not on the list of properties with resolved values:
https://drafts.csswg.org/cssom-1/#resolved-values

This used to be more useful when our buttons and so on didn't respect
the UA sheet borders.

Let's try to do this? Otherwise we should file a bug to change the spec.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ff91edfd666
Make ProgressBar appearance honor UA sheet borders. r=dholbert

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/477f5c91a780
Do not return used border-width values from getComputedStyle(). r=layout-reviewers,dholbert

Comment 11

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

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

Comment 12

[amarc]

https://hg.mozilla.org/mozilla-central/rev/1ff91edfd666
https://hg.mozilla.org/mozilla-central/rev/477f5c91a780

Comment 13

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

Upstream PR merged by moz-wptsync-bot