Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-ui] Add caret/menulist-textfield/menulist-text appearance values #3529

Closed

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Jan 18, 2019

This is an alias to 'none' for better web compat (compared to not
supporting the value at all or making it equivalent to 'menulist').

See https://bugzilla.mozilla.org/show_bug.cgi?id=1500423#c62

@zcorpan zcorpan changed the title [css-ui] Add 'menulist-textfield' appearance value [css-ui] Add 'caret' and 'menulist-textfield' appearance values Jan 18, 2019
@MatsPalmgren
Copy link

I disagree that menulist-textfield is best implemented as none for web-compatibility. In Firefox and Chrome (which both support this value), it renders exactly the same as textfield but without rendering the border. So I believe that making it equivalent to textfield is much better than none.

Regarding caret - is there any evidence that this value is needed at all for web-compatibility? (and if so, do you have an example on what it is used for?)

@zcorpan
Copy link
Member Author

zcorpan commented Jan 21, 2019

Making it like textfield but not rendering the border is indeed demonstrated by shipping. I just think that the "suppress rendering the border" is pretty weird and since this value has very low usage, I wanted to check if it could be implemented as an alias to none instead without further magic. My analysis suggests that it ought to be.

caret has relatively high usage (0.28%):

https://www.chromestatus.com/metrics/feature/timeline/popularity/1355

I don't know if it is really needed, it just seemed like a safer option to support it. Looking at examples from httparchive it seems it's mostly used on text fields to "remove the border", but they typically also set border: none so it would imply appearance: none anyway if the value was not supported.

@MatsPalmgren
Copy link

I think it's an invalid assumption that not painting the border on a textfield renders the same as appearance: none. The former is still rendered based on the platform theme, whereas the latter isn't. They may have different colors and size for example, so they are not at all equivalent.

Thanks for the info on caret. Does that mean that an UA could treat caret as menulist-textfield internally? It sounds like they would render exactly the same.

@@ -1795,7 +1795,7 @@ so that CSS can be used to restyle them.

<pre class="propdef">
Name: appearance
Value: ''appearance/none'' | ''auto'' | ''button'' | <<compat>>
Value: ''appearance/none'' | ''caret'' | ''menulist-textfield'' | ''menulist-text'' | ''auto'' | ''button'' | <<compat>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest grouping these values in another non terminal, like <<compat>>. Maybe call them <<compat-auto>> and <<compat-none>> to distinguish. That way, the propdef value definition emphasizes to authors the main values to be used manually, and all the rest that's needed to behave one way or another for compat reasons is specified, without drawing too much attention to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@frivoal
Copy link
Collaborator

frivoal commented Jan 23, 2019

The legacy behavior of appearance in general is overly complicated, so to the extent that compat allows, I am supportive of making more values be exact equivalents of none or auto. Even if they currently have some minor side effect that makes them different from auto or none, if we can get rid of that without breaking the web, that sounds like a good thing to me (for the sake of authors' sanity, and cost of maintenance costs or interop testing)

In this case, can we? @MatsPalmgren points out that there is a side effect, so we should at least be careful, but @zcorpan's research seems to indicate that by and large people don't depend on that difference. @zcorpan can you share a bit more about what led you to that conclusion?

@frivoal frivoal added the css-ui-4 Current Work label Jan 23, 2019
@frivoal
Copy link
Collaborator

frivoal commented Jan 23, 2019

@zcorpan can you share a bit more about what led you to that conclusion?

This comment in bugzilla is what I was looking for: https://bugzilla.mozilla.org/show_bug.cgi?id=1500423#c62

Your proposal seems safe to me indeed. @MatsPalmgren does that work for you?

This is an alias to 'none' for better web compat (compared to not
supporting the value at all or making it equivalent to 'menulist').

See https://bugzilla.mozilla.org/show_bug.cgi?id=1500423#c62

[css-ui] Add 'caret' appearance value

This value is supported cross-browser, but does different things.
In EdgeHTML/WebKit/Chromium, it is like 'none' but the border is
not painted. In Gecko, it is like 'none'.

Chrome Use counter for this value is ~0.266%.
https://www.chromestatus.com/metrics/feature/timeline/popularity/1355

Add menulist-text as an alias to none

Use <compat-none> and <compat-auto>
@zcorpan zcorpan force-pushed the zcorpan/appearance-menulist-textfield branch from 7f61c0e to 5d93bc2 Compare January 23, 2019 10:07
@zcorpan
Copy link
Member Author

zcorpan commented Jan 23, 2019

I've addressed @frivoal's comment and rebased.

Also cc @tkent-google - do you think this change is acceptable/good/bad?

@zcorpan
Copy link
Member Author

zcorpan commented Jan 23, 2019

I think 'listbox' can be an alias to 'none' as well. A <select multiple> seems like it can be implemented with just CSS, and specifying -webkit-appearance: listbox on a drop-down select would make it act like none. Am I missing something?

@zcorpan
Copy link
Member Author

zcorpan commented Jan 23, 2019

As for 'caret', apparently it used to render the border and this changed (in 2014?) to not render the border, which regressed rendering according to this:

https://stackoverflow.com/questions/23557729/webkit-appearance-caret-removes-input-border-from-android-and-windows-browser

(found via https://bugzilla.mozilla.org/show_bug.cgi?id=1481632 )

@tkent-google
Copy link
Contributor

Also cc @tkent-google - do you think this change is acceptable/good/bad?

Please hold this change.
Chrome 72 will have a counter for rendering of all unimplemented appearance keywords such as caret and menulist-textfield. If the counter value is low, Chromium project will try to remove all such values.
https://www.chromestatus.com/metrics/feature/timeline/popularity/2614

https://www.chromestatus.com/metrics/feature/timeline/popularity/1355 counts in CSS parsing. It counts unused rule sets, and It is not appropriate for evaluating the removal impact.

@zcorpan zcorpan changed the title [css-ui] Add 'caret' and 'menulist-textfield' appearance values [css-ui] Add caret/menulist-textfield/menulist-text appearance values Jan 24, 2019
@zcorpan
Copy link
Member Author

zcorpan commented Jan 30, 2019

Per https://www.chromestatus.com/features/schedule 72 reached stable yesterday, but I suppose it needs a few more days before the stats are accurate?

If the counter value is low, Chromium project will try to remove all such values.

OK, but the values specified in this PR all have some usage based on httparchive analysis, in some cases on select elements, with the expectation to have the dropdown arrow removed (and apply custom styles).

Examples:

  • select{-webkit-appearance:caret;background-image:url("images/icon_arrow_down_blue.svg");background-position:95% center;background-repeat:no-repeat;-webkit-background-size:16px 16px;-moz-background-size:16px 16px;background-size:16px 16px;outline:none}
  • .select-wrapper .select-inner select{position:absolute;z-index:999;top:0;left:0;height:100%;width:100%;margin:0;padding:0;border:0;opacity:0;background:url("/content/images/select_bknd_transparent.png");visibility:visible;-webkit-appearance:menulist-text}

Not parsing these values at all would have such select elements either have menulist or menulist-button appearance, breaking the intended custom style. Therefore I think it is less likely to break such web pages if the keywords in this PR are aliases to none.

@tkent-google
Copy link
Contributor

Per https://www.chromestatus.com/features/schedule 72 reached stable yesterday, but I suppose it needs a few more days before the stats are accurate?

We don't push Chrome 72 to all clients yet. It needs a few days after all clients are updated.

Both of removing the keywords and aliasing them to none have site compatibility risk. The former would show the default appearance unexpectedly, and the latter would show border unexpectedly. If both of their risk are low and we can do either of them, we should take the former to avoid to keep the meaningless keywords forever.

@zcorpan
Copy link
Member Author

zcorpan commented May 15, 2019

Per https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/ldH_uIfk_QE/Ua0bGyLIAwAJ the caret, menulist-textfield and menulist-text values will be removed in Chromium 75, which is scheduled to reach stable around June 4.

With the assumption that the change will stick and there are no objections, I'll update this PR and the tests to match.

The remaining change in this PR is that listbox is an alias to none.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/6909

@zcorpan
Copy link
Member Author

zcorpan commented May 15, 2019

The change in Chromium removes these keywords that are currently in the spec or in this PR

  • button-bevel
  • caret
  • menulist-text
  • menulist-textfield

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request May 15, 2019
@zcorpan
Copy link
Member Author

zcorpan commented May 17, 2019

This PR is now superseded by #3942

@frivoal frivoal added Closed as Duplicate Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. labels Dec 30, 2019
@frivoal frivoal added this to Done in appearance Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed as Duplicate Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-ui-4 Current Work Tracked in DoC
Projects
appearance
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants