Define button layout by zcorpan · Pull Request #4143 · whatwg/html (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation30 Commits12 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This specifies the layout model of buttons (the button element, the input element in the Button, Reset, Submit states, and the button part of input in the File Upload state).

Fixes #1024. Fixes #2948. Part of #4081 and #4082.

Tests: TODO

@MatsPalmgren can you please review this and comment if anything is different from Gecko but shouldn't be?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes looks fine to with the nits below addressed as you see fit.

BTW, it's a bit hard to review a patch file on HTML markup like this, and I don't really have much time to review individual changes like this, so it's probably better if I give feedback on the final document instead (or at least somewhat complete snapshots thereof).

The `button` element expected to render as an 'inline-block' box
depicting a button whose contents are the contents of the element.
The `button` element, when it generates a box (i.e., is not 'display: none' or
'display: contents') and the conditions below do not both apply is expected to depict a button

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "expected to depict a button" is too general. You can make it depict various things using -webkit-appearance in current crop of UAs... although perhaps we should restrict some of that. Depends on a spec for "which elements should each of the -webkit-appearance values apply to".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would need to revisit this when the appearance spec can be referenced. For now "depict a button" is no change from the previous text.

source Show resolved Hide resolved

palette) from which the color can be changed. The element is expected to use button
layout, that has no child boxes of the anonymous button content box. The
anonymous button content box is expected to have a <span data-x="presentational
hints">presentational hint setting the 'background-color' property to the

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also depends on -webkit-appearance.

@@ -113251,11 +113323,14 @@ details[open] > summary {
The `input` element as a file upload control

An `input` element whose `type` attribute is in
the File Upload state is expected to render as an
the File Upload state, when it generates a box (i.e.,
is not 'display: none' or 'display: contents'), is expected to render as an
'inline-block' box containing a span of text giving the file name(s) of the <span
data-x="concept-input-type-file-selected">selected files, if any, followed by a button

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"file name(s) ... followed by a button" -- the button is typically before the file name(s), but the spec should probably be worded to not make any assumptions about placement. Also, the rendering is affected by -webkit-appearance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was referenced

Jan 7, 2019

This matches WebKit/Chromium and is consistent with other buttons.

Fixes #4281.

@MatsPalmgren

BTW, it's a bit hard to review a patch file on HTML markup like this

PRs in whatwg/html (and several other repos) have generated HTML diffs, linked from the description. Does that help?

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request

Jan 11, 2019

This was referenced

Jan 11, 2019

Per w3c/csswg-drafts#3226 (comment) I will remove the "display: inline and appearance: none disables button layout" for now, so we can discuss other ways to disable button layout (a new property or new appearance value or something).

@annevk , @domenic, I think this and the tests can be merged. It got left in an unmerged state as I was focusing on other tasks for a while.

Can either of you do an editorial review + sanity check? I can report browser bugs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a big improvement from the status quo. I have a number of questions, most of the form "is this formally defined anywhere we could xref", that are mostly reflective of my unfamiliarity with this space. But hopefully they're helpful.

Given the improvement, I'm happy to get this merged and consider none of my comments blocking. Please address as many or as few as makes sense to you.

If the computed value of 'display' is 'inline-grid', 'grid',
'inline-flex', or 'flex', then behave as the computed value.

Otherwise, if the computed value of 'display' is inline-level, then

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason inline-level is not quoted?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline-level is not a value, but that means this sentence is skipping a level of indirection.

It should probably say something like "if the computed value of 'display' is a value such that the outer display type is 'inline'"




The element is expected to establish a new formatting context for its contents. The type

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we xref "formatting context" to something?

The element is expected to establish a new formatting context for its contents. The type
of this formatting context is determined by its 'display' value, as usual.

If the element is absolutely positioned, then for the purpose of the CSS visual

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "absolutely positioned" formally defined anywhere we could link to? (If not, oh well.)

of this formatting context is determined by its 'display' value, as usual.

If the element is absolutely positioned, then for the purpose of the CSS visual
formatting model, act as if the element is a replaced element.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref replaced element

If the element is absolutely positioned, then for the purpose of the CSS visual
formatting model, act as if the element is a replaced element.

The element's box shrink-wraps.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The element's box shrink-wraps.

For the purpose of the 'normal' keyword of the 'align-self' property, act
as if the element is a replaced element.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref replaced element

following behaviors:


The 'display' property is set to 'flow-root'.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "set to" sufficiently precise? (It may be, I'm not a CSS expert.) It seems different than other parts of the spec which talk about computed values or presentation hints or similar.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know if it's precise, and I don't know of anything to link to. It's different, but this is an anonymous box -- we get to say what it is more directly, and the property is not directly observable. But I've changed this to say what the effect is on the box, instead.

layout, that has no child boxes of the anonymous button content box. The
anonymous button content box is expected to have a <span data-x="presentational
hints">presentational hint setting the 'background-color' property to the
element's value.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Color well does not have the "when it generates a box (i.e., is not display: none or display: contents)" that the others have. Is that intentional? Is that sentence necessary in general---perhaps it could be factored out?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's necessary in that we don't want to require things like "behave as display: inline-block" on elements that are display: none. The omission here was not intentional. I'll file an issue on factoring it out.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the additional info! LGTM now.

This was referenced

May 13, 2019

OK, I have now filed bugs (see OP). Let's merge.

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request

May 14, 2019

Follows whatwg/html#4143.

(More tests will be added to this PR.)

Test grid/inline-grid on button

Test flex/inline-flex on button

Test inline-level (except ruby)

Test other display values on button

Add a clarifying comment

Test non-auto left/right on abspos button

Test that buttons shrink-wrap

Test propagation of text-decoration into buttons

Test aspects of the anonymous button content box

zcorpan deleted the zcorpan/button-layout branch

May 14, 2019 14:11

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request

May 14, 2019

The following features are defined in the CSS Flexible Box Layout
specification:


The 'align-self' property

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.



The term <dfn
data-x-href="block-level" title="undefined" rel="noopener noreferrer">https://drafts.csswg.org/css-display/#block-level">block-level is defined in
The following terms and features are defined in theis defined in