HTML: Add a tentative test for input element's baseline alignment #20306

zcorpan · 2019-11-18T22:40:53Z

domenic · 2019-11-19T18:30:35Z

I'd like someone more familiar with this work to be the reviewer if possible, so I've un-assigned myself. If finding such reviewers is hard though, I can accept the role thrust upon me by wpt-pr-bot and do the review. Just let me know.

zcorpan · 2020-01-27T14:17:59Z

@domenic nobody has stepped up to review. :( Can you review?

I recommend reading the comments in whatwg/html#4082 - especially those with the word "baseline".

domenic

So, I vaguely get what this test is doing, and am happy to approve it, especially seeing the 100% pass rate in Firefox and how it reveals useful differences in other browsers. Perhaps we should just merge it given how long it's been in limbo. But I think there are some opportunities for making it clearer, so if you'll indulge me...

The biggest improvements would come from "unrolling" some of the setup code. There's too much logic there at the moment. Here's one route to reducing it:

Do the refs table entirely in HTML, with no JS setup. It looks like right now the only JS setup is for the image URL, but I'm not sure, because there's so much setup code. I don't currently have a sense of what the refs are going to look like when all is done.
Add a comment explaining that the refs table is not meant to match the test table. It's meant to allow you to grab the offsetTop values from the span
Create the tests by:
- Creating one set of cells in the HTML, with no style attributes. Have them totally set up and readable from the source. This allows you to remove the setup logic on lines 97-107 which translate between inputTypes, which are not really input types, and various attributes set on <input> elements.
- Have the two nested looks (appearance/overflow) clone the base test table, and set style on the input contained therein.
- Consider keeping the creation of the test table inside the test that tests it. Right now the loop that tells you what the actual intreesting factors are (appearance + overflow) is far separated from the promise_test() call.

Hope this helps!

html/rendering/widgets/baseline-alignment-and-overflow.tentative.html

domenic · 2020-01-27T21:49:32Z

html/rendering/widgets/baseline-alignment-and-overflow.tentative.html

+    for (const row of testTable.children) {
+      const input = row.firstChild.lastElementChild;
+      const allowedDelta = 3;
+      promise_test(async () => {


Why is this a promise_test?

Because with promise_setup() only promise_test() is allowed (see step 6 in https://github.com/web-platform-tests/rfcs/blob/master/rfcs/asynchronous_setup.md ).

Could you add a comment explaining that?

See whatwg/html#5065 whatwg/html#4840

zcorpan · 2020-01-28T15:01:05Z

Thanks, @domenic! I've tried to improve things based on your suggestions. Reading your comment again, I realize it's not exactly what you suggested, and the promise_test is still separated from the creation of the table. I hope it's clear enough now, though. 🙂

domenic

This helps a lot, thanks! I think I can now totally understand what is being tested, and most people readnig this should be able to do so too. What remains is that I have a hard time understanding why we're expecting these particular assertions to hold; I wrote a longer comment asking about that.

domenic · 2020-01-29T22:19:49Z

html/rendering/widgets/baseline-alignment-and-overflow.tentative.html

+    for (const row of testTable.children) {
+      const input = row.firstChild.lastElementChild;
+      const allowedDelta = 3;
+      promise_test(async () => {


Could you add a comment explaining that?

html/rendering/widgets/baseline-alignment-and-overflow.tentative.html

domenic · 2020-01-29T22:30:13Z

html/rendering/widgets/baseline-alignment-and-overflow.tentative.html

+<h2>refs</h2>
+<!--
+The first span's offsetTop is what we want to compare with the corresponding test's span's offsetTop.
+The sibling element is there to control where the baseline for the line box will be.


It's still not really clear to me why, for example, the offsetTop of a span that sits next to an <input type=range> should be the same as that of a span that sits next to an <img> with a valid src="" attribute (refImgSrcOffsetTop).

Or why the offsetTop of a span that sits next to an <input type=checkbox> should be the same as that of a span that sits next to an <img> with a valid src="" attribute and no margin-bottom (which is for some reason called refCheckboxOffsetTop instead of refImgSrcNoMarginBottomOffsetTop).

Or why the offsetTop of a span that sits next to an <input type=tel> should be the same as that of a span that sits next to another span which uses display: inline-grid; border: 2px solid; align-items: center;.

In other words, where do these elements on the right, like img with src, or img with src and no margin bottom, or a span with very particular CSS properties set, come from? Does this correspond to some CSS concept?

Maybe this would become clearer if there were a spec. Probably the plan is not to write the spec as something literally like "the offsetTop for a span that sits next to an <input type=range> must match the offsetTop of a span that sits next to an <img> with a valid src="" attribute"? What would the spec be? I gather from the test title that we're testing "baseline alignment", and I guess offsetTop is a way of measuring that?

The spec would say where the baseline is for each widget. See whatwg/html#5065

I've added a comment trying to explain what's going on with this test.

(which is for some reason called refCheckboxOffsetTop instead of refImgSrcNoMarginBottomOffsetTop)

The variable names are intended to say which widget they emulate, not how the fake widget is implemented.

The spec would say where the baseline is for each widget. See whatwg/html#5065

This issue didn't clarify anything for me. It says that the spec should specify a baseline. (Thankfully the new comment you added clarifies that this is related to offsetTop.) It doesn't say what that baseline will be, or why type=tel should correspond to span with display: inline-grid, border 2px solid, align-items center.

The variable names are intended to say which widget they emulate, not how the fake widget is implemented.

Is the idea that a span with display: inline-grid, border 2px solid, align-items center is something like a text widget? How is the text widget/span-with-those-properties category, or the checkbox widget/img-with-valid-src-and-no-margin-bottom category, related to the four categories you list in lines 24-27?

Will the spec eventually talk about those four categories, or will it talk about spans-with-those-properties, or...? Why are there 6 refs, but 4 categories?

Is the idea that a span with display: inline-grid, border 2px solid, align-items center is something like a text widget?

Yes.

How is the text widget/span-with-those-properties category, or the checkbox widget/img-with-valid-src-and-no-margin-bottom category, related to the four categories you list in lines 24-27?

I've clarified this now.

Will the spec eventually talk about those four categories, or will it talk about spans-with-those-properties, or...? Why are there 6 refs, but 4 categories?

The new comment could be used as a starting point for writing the spec, I think!

Thanks for pushing, here. A test that is confusing isn't helping anyone. 🙂

domenic

Awesome, thank you very much!

The only potential tweak I would make is renaming the offsetTop variables or table rows to make the connection 100% clear. (Although it is a lot easier to connect the dots now.) E.g. it is not obvious that "ref for image src" is meant to be a ref for category 6, of which an-img-with-src-attribute is only a representative sample.

This might be tricky given that you don't have names for the categories yet, but here are some ideas:

text-input-like, checkbox-input-like, color-input-like, file-input-like, img-alt-with-visible-overflow-like, and img-like. (The "-like", or similar, is important, to emphasize that these are representatives of the class.)
inline-block with mods, border-box, content-box, inline-block with button, inline-block, replaced elements

Either way, this is definitely clear enough to merge, either before or after such suggestions. Thanks for taking my feedback in stride, and teaching me about this area.

zcorpan · 2020-01-31T08:36:24Z

Thanks! Fixed. Some of the names got a bit long, but ¯\(ツ)/¯

zcorpan requested a review from MatsPalmgren November 18, 2019 22:40

wpt-pr-bot added the html label Nov 18, 2019

wpt-pr-bot assigned domenic Nov 18, 2019

wpt-pr-bot requested review from annevk, domenic, foolip, jdm, jgraham and zqzhang November 18, 2019 22:41

zcorpan mentioned this pull request Nov 18, 2019

Make all input elements display: inline-block by default whatwg/html#4840

Merged

3 tasks

domenic removed their assignment Nov 19, 2019

wpt-pr-bot assigned annevk Nov 19, 2019

annevk removed their assignment Nov 20, 2019

gsnedders closed this Jan 24, 2020

gsnedders deleted the zcorpan/input-baseline-alignment branch January 24, 2020 18:08

gsnedders restored the zcorpan/input-baseline-alignment branch January 24, 2020 18:53

Hexcles reopened this Jan 24, 2020

domenic reviewed Jan 27, 2020

View reviewed changes

zcorpan added 5 commits January 28, 2020 15:08

HTML: Add a tentative test for input element's baseline alignment

0114dd7

See whatwg/html#5065 whatwg/html#4840

Remove stray end tag

77d5983

Clarify how the references work

507bc23

Use promise_setup()

4dd4b44

Use early return

dc9769e

zcorpan force-pushed the zcorpan/input-baseline-alignment branch from 07085ca to dc9769e Compare January 28, 2020 14:16

wpt-pr-bot assigned zcorpan Jan 28, 2020

zcorpan added 2 commits January 28, 2020 15:21

Use only markup for the references

1b5d312

Apply domenic's suggestions

802f8fb

domenic reviewed Jan 29, 2020

View reviewed changes

zcorpan added 2 commits January 30, 2020 15:36

Address comments

0270695

Specify where the baseline should be for each widget

fd27a3f

domenic approved these changes Jan 30, 2020

View reviewed changes

Use clear names like refImageInputShowingAltOverflowVisibleLikeOffsetTop

82f9b76

domenic approved these changes Jan 31, 2020

View reviewed changes

zcorpan closed this Feb 2, 2020

zcorpan reopened this Feb 2, 2020

zcorpan merged commit d472ae3 into master Feb 3, 2020

zcorpan deleted the zcorpan/input-baseline-alignment branch February 3, 2020 02:00

zcorpan mentioned this pull request Feb 3, 2020

Specify where text baseline is for form controls / widgets whatwg/html#5065

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML: Add a tentative test for input element's baseline alignment #20306

HTML: Add a tentative test for input element's baseline alignment #20306

zcorpan commented Nov 18, 2019

domenic commented Nov 19, 2019

zcorpan commented Jan 27, 2020

domenic left a comment

domenic Jan 27, 2020

zcorpan Jan 28, 2020

domenic Jan 29, 2020

zcorpan Jan 30, 2020

zcorpan commented Jan 28, 2020

domenic left a comment

domenic Jan 29, 2020

domenic Jan 29, 2020 •

edited

Loading

zcorpan Jan 30, 2020

zcorpan Jan 30, 2020

domenic Jan 30, 2020

zcorpan Jan 30, 2020

domenic left a comment •

edited

Loading

zcorpan commented Jan 31, 2020 •

edited

Loading

HTML: Add a tentative test for input element's baseline alignment #20306

HTML: Add a tentative test for input element's baseline alignment #20306

Conversation

zcorpan commented Nov 18, 2019

domenic commented Nov 19, 2019

zcorpan commented Jan 27, 2020

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcorpan commented Jan 28, 2020

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment • edited Loading

Choose a reason for hiding this comment

zcorpan commented Jan 31, 2020 • edited Loading

domenic Jan 29, 2020 •

edited

Loading

domenic left a comment •

edited

Loading

zcorpan commented Jan 31, 2020 •

edited

Loading