1703630 - Update reftests that break when using non-native menulists or checkboxes, when browser.proton.enabled is true and remove non-proton toolkit CSS blocks

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

In bug 1703623, we made some reftests use native, old-style menulists in order to pass. We also disabled browser.proton.enabled for some checkbox ones that don't pass, regardless of the native=true state.

We should invest some time here in figuring out what's going wrong here, and get these tests running again in the same configuration that Nightly is running in.

Comment 1

[:Gijs (he/him)]

Michelle kindly pushed this to try: https://treeherder.mozilla.org/jobs?repo=try&revision=a7fd5930b4656ab55507e2e6f29655c950241ccd

There are 3 failing tests:

checkboxsize.xhtml - failing log
nostretch.xhtml - failing log
baseline.xhtml - failing log

AFAICT the nostretch and baseline ones are both to do with menulists and proton increasing the vertical size of the menulist.

The checkbox one appears to be due to the font-size styles on the checkbox no longer influencing the size of the resulting checkbox.

I'm honestly not 100% sure what we want to do about this. For the checkboxes, I'm not sure if we want the font size to continue to influence checkbox size (that doesn't have native=true), in which case perhaps just adding native=true is fine? The nostretch case says:

 * This test tests whether you can put different widgets in the same
 * hbox without stretching them, even if you don't set align="center".
 * I.e. prior to the fix that added this patch, having a button and a
 * menulist in the same hbox next to each other would stretch the menulist
 * vertically because the button had such a big vertical margin.

and AFAICT the failure is because in fact the opposite problem to the one the test describes is what occurs: the button gets bigger because the menulist is bigger. I don't think this is a proton issue in the sense that we haven't broken XUL layout or whatever logic this is describing, but the test is failing so I'm not sure what to do about that. Do we just whack native=true on these, too? Or is there some other fix required here?

Markus, do you have an intuition about what the right thing is here?

Comment 2

[Markus Stange [:mstange]]

(In reply to :Gijs (out; back Jun 21; he/him) from comment #1)

The checkbox one appears to be due to the font-size styles on the checkbox no longer influencing the size of the resulting checkbox.

I'm honestly not 100% sure what we want to do about this. For the checkboxes, I'm not sure if we want the font size to continue to influence checkbox size (that doesn't have native=true), in which case perhaps just adding native=true is fine?

The test was originally written for native checkboxes, so adding native=true seems fine.

I don't think this is a proton issue in the sense that we haven't broken XUL layout or whatever logic this is describing,

The test wasn't written to test layout logic, it was written to test the default toolkit styles, from the perspective of a non-Firefox XUL user. This perspective is largely irrelevant today. But the idea was: If I slap together some XUL to make a dialog box with some contents, in the most straightforward way possible, will the result look good by default?
So in this case, the test has found out that, if I put a menulist and a button in the same hbox, the button will not look good by default.
Whether you consider that a problem is up to you.

but the test is failing so I'm not sure what to do about that. Do we just whack native=true on these, too? Or is there some other fix required here?

You could probably even remove the test entirely. These days we only care about actual uses in the Firefox UI looking good.

Comment 3

[Michelle Goossens [:masterwayz]]

When this gets fixed, we will need to remove the not -moz-proton CSS rules in toolkit/themes in the checkbox.css and menulist.css files. I cannot do this in Bug 1714462 as otherwise I will hit this bug on the OS reftests.

Comment 4

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #2)

(In reply to :Gijs (out; back Jun 21; he/him) from comment #1)

The checkbox one appears to be due to the font-size styles on the checkbox no longer influencing the size of the resulting checkbox.

I'm honestly not 100% sure what we want to do about this. For the checkboxes, I'm not sure if we want the font size to continue to influence checkbox size (that doesn't have native=true), in which case perhaps just adding native=true is fine?

The test was originally written for native checkboxes, so adding native=true seems fine.

Alright, let's do this.

I don't think this is a proton issue in the sense that we haven't broken XUL layout or whatever logic this is describing,

The test wasn't written to test layout logic, it was written to test the default toolkit styles, from the perspective of a non-Firefox XUL user. This perspective is largely irrelevant today. But the idea was: If I slap together some XUL to make a dialog box with some contents, in the most straightforward way possible, will the result look good by default?
So in this case, the test has found out that, if I put a menulist and a button in the same hbox, the button will not look good by default.
Whether you consider that a problem is up to you.

Yeah, I'm going to say that we should stop caring about this. If anything, we'll be moving to CSS flexbox, away from XUL, and perhaps to HTML for the widgets themselves (either vanilla or wrapped in HTML-based custom element convenience wrappers). Either way, I think these tests have outlived their usefulness, and fixing any styling issues should be trivial with some light CSS.

but the test is failing so I'm not sure what to do about that. Do we just whack native=true on these, too? Or is there some other fix required here?

You could probably even remove the test entirely. These days we only care about actual uses in the Firefox UI looking good.

Right. Let's do that.

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1703630 - update mac reftests for proton and remove last proton media queries from toolkit, r?harry

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/664f97f70341
update mac reftests for proton and remove last proton media queries from toolkit, r=jaws

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/664f97f70341