Bug 238930 – [css-ui] Remove some unimplemented -webkit-appearance keywords

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

Affected WPT test -

imported/w3c/web-platform-tests/css/css-ui/appearance-cssom-001.html

Created attachment 456919 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/91438814>

[Aditya Keerthi]

Comment on attachment 456919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456919&action=review

> Source/WebCore/css/CSSPrimitiveValueMappings.h:517
>      case MenulistTextPart:

I think we can remove `MenulistTextPart` too. I don't see it used anywhere.

> LayoutTests/imported/blink/editing/execCommand/outdent-collapse-table-crash.html:6
> +    -webkit-appearance: meter;

Is `-webkit-appearance` relevant to this test? Since `media-controls-background` was unimplemented, I wonder if we can just remove the `-webkit-appearance` entirely.

Created attachment 457032 [details]
Patch

[Tim Nguyen (:ntim)]

Seems like iOS needs rebaselining: https://ews-build.s3-us-west-2.amazonaws.com/iOS-15-Simulator-WK2-Tests-EWS/457032-12754-rerun/imported/w3c/web-platform-tests/css/css-ui/appearance-cssom-001-pretty-diff.html

Created attachment 457237 [details]
Patch

[Patrick Angle]

Comment on attachment 457237 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457237&action=review

> Source/WebInspectorUI/UserInterface/External/CSSDocumentation/CSSDocumentation.js:32
> +        "syntax": "none | button | caret | checkbox | default-button | inner-spin-button | listbox | listitem | media-play-button | media-slider | media-sliderthumb | media-volume-slider | media-volume-sliderthumb | menulist | menulist-button | meter | progress-bar | progress-bar-value | push-button | radio | searchfield | searchfield-cancel-button | searchfield-decoration | searchfield-results-button | searchfield-results-decoration | slider-horizontal | slider-vertical | sliderthumb-horizontal | sliderthumb-vertical | square-button | textarea | textfield | -apple-pay-button"

This file is generated by` Source/WebInspectorUI/Scripts/update-inspector-css-documentation`. To override the data here, please add an entry to `Source/WebInspectorUI/UserInterface/External/CSSDocumentation/CSSDocumentation-overrides.json` for this properties' `syntax` as well so that a future update does not clobber this change.

Created attachment 457515 [details]
Patch

(In reply to Patrick Angle from comment #7)
> Comment on attachment 457237 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457237&action=review
> 
> > Source/WebInspectorUI/UserInterface/External/CSSDocumentation/CSSDocumentation.js:32
> > +        "syntax": "none | button | caret | checkbox | default-button | inner-spin-button | listbox | listitem | media-play-button | media-slider | media-sliderthumb | media-volume-slider | media-volume-sliderthumb | menulist | menulist-button | meter | progress-bar | progress-bar-value | push-button | radio | searchfield | searchfield-cancel-button | searchfield-decoration | searchfield-results-button | searchfield-results-decoration | slider-horizontal | slider-vertical | sliderthumb-horizontal | sliderthumb-vertical | square-button | textarea | textfield | -apple-pay-button"
> 
> This file is generated by`
> Source/WebInspectorUI/Scripts/update-inspector-css-documentation`. To
> override the data here, please add an entry to
> `Source/WebInspectorUI/UserInterface/External/CSSDocumentation/
> CSSDocumentation-overrides.json` for this properties' `syntax` as well so
> that a future update does not clobber this change.

Thanks very much for the suggestion. The patch has been updated. Is it Okay to take another look at it?

[Devin Rousso]

Comment on attachment 457515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457515&action=review

> Source/WebInspectorUI/UserInterface/External/CodeMirror/css.js:-595
> -    "both", "bottom", "break", "break-all", "break-word", "bullets", "button", "button-bevel",

please undo all the changes in `Source/WebInspectorUI/UserInterface/External` (except for `CSSDocumentation.js` and `CSSDocumentation-overrides.json`) as these files come from elsewhere and we don't want to have to remember to make this change every time we update those files

in this case, I'd file an issue (or PR) on <https://github.com/codemirror/CodeMirror> to have these removed.  We can then update this file from that repo

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:-1014
> -        "none", "checkbox", "radio", "push-button", "square-button", "button", "button-bevel", "default-button", "inner-spin-button", "listbox", "listitem", "media-controls-background", "media-controls-dark-bar-background", "media-controls-fullscreen-background", "media-controls-light-bar-background", "media-current-time-display", "media-enter-fullscreen-button", "media-exit-fullscreen-button", "media-fullscreen-volume-slider", "media-fullscreen-volume-slider-thumb", "media-mute-button", "media-overlay-play-button", "media-play-button", "media-return-to-realtime-button", "media-rewind-button", "media-seek-back-button", "media-seek-forward-button", "media-slider", "media-sliderthumb", "media-time-remaining-display", "media-toggle-closed-captions-button", "media-volume-slider", "media-volume-slider-container", "media-volume-slider-mute-button", "media-volume-sliderthumb", "menulist", "menulist-button", "menulist-text", "menulist-textfield", "meter", "progress-bar", "progress-bar-value", "slider-horizontal", "slider-vertical", "sliderthumb-horizontal", "sliderthumb-vertical", "caret", "searchfield", "searchfield-decoration", "searchfield-results-decoration", "searchfield-results-button", "searchfield-cancel-button", "snapshotted-plugin-overlay", "textfield", "relevancy-level-indicator", "continuous-capacity-level-indicator", "discrete-capacity-level-indicator", "rating-level-indicator", "-apple-pay-button", "textarea", "attachment", "borderless-attachment", "caps-lock-indicator",

as a FYI, we normally don't remove things like this from the Web Inspector frontend because Web Inspector is able to inspect older backends which may still have support for the thing (even though it's been removed for all new places)

in this case, however, I think it's probably fine as I doubt these properties were all that common (or useful)

https://github.com/codemirror/CodeMirror/pull/6912

Created attachment 458357 [details]
Patch

(In reply to Devin Rousso from comment #10)
> Comment on attachment 457515 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457515&action=review
> 
> > Source/WebInspectorUI/UserInterface/External/CodeMirror/css.js:-595
> > -    "both", "bottom", "break", "break-all", "break-word", "bullets", "button", "button-bevel",
> 
> please undo all the changes in
> `Source/WebInspectorUI/UserInterface/External` (except for
> `CSSDocumentation.js` and `CSSDocumentation-overrides.json`) as these files
> come from elsewhere and we don't want to have to remember to make this
> change every time we update those files
> 
> in this case, I'd file an issue (or PR) on
> <https://github.com/codemirror/CodeMirror> to have these removed.  We can
> then update this file from that repo
> 
> 
Thanks! PR https://github.com/codemirror/CodeMirror/pull/6912 has been merged.

> > Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:-1014
> > -        "none", "checkbox", "radio", "push-button", "square-button", "button", "button-bevel", "default-button", "inner-spin-button", "listbox", "listitem", "media-controls-background", "media-controls-dark-bar-background", "media-controls-fullscreen-background", "media-controls-light-bar-background", "media-current-time-display", "media-enter-fullscreen-button", "media-exit-fullscreen-button", "media-fullscreen-volume-slider", "media-fullscreen-volume-slider-thumb", "media-mute-button", "media-overlay-play-button", "media-play-button", "media-return-to-realtime-button", "media-rewind-button", "media-seek-back-button", "media-seek-forward-button", "media-slider", "media-sliderthumb", "media-time-remaining-display", "media-toggle-closed-captions-button", "media-volume-slider", "media-volume-slider-container", "media-volume-slider-mute-button", "media-volume-sliderthumb", "menulist", "menulist-button", "menulist-text", "menulist-textfield", "meter", "progress-bar", "progress-bar-value", "slider-horizontal", "slider-vertical", "sliderthumb-horizontal", "sliderthumb-vertical", "caret", "searchfield", "searchfield-decoration", "searchfield-results-decoration", "searchfield-results-button", "searchfield-cancel-button", "snapshotted-plugin-overlay", "textfield", "relevancy-level-indicator", "continuous-capacity-level-indicator", "discrete-capacity-level-indicator", "rating-level-indicator", "-apple-pay-button", "textarea", "attachment", "borderless-attachment", "caps-lock-indicator",
> 
> as a FYI, we normally don't remove things like this from the Web Inspector
> frontend because Web Inspector is able to inspect older backends which may
> still have support for the thing (even though it's been removed for all new
> places)
> 
> in this case, however, I think it's probably fine as I doubt these
> properties were all that common (or useful)


Just in case, I have checked out changes on this file.

[Tim Nguyen (:ntim)]

Is this ready for landing? If so, please set commit-queue+

Devin, does the latest update look Okay to you? Thanks!

[Devin Rousso]

(In reply to zsun from comment #15)
> Devin, does the latest update look Okay to you? Thanks!

yeah looks good :)

i'll chat with some folks internally to see about updating CodeMirror

[EWS]

Committed r293511 (250042@main): <https://commits.webkit.org/250042@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458357 [details].

[Philippe Normand]

This is the first in a suite of patches breaking media controls on WPE/GTK :( After this patch landed, the enter-fullscreen and volume button are now painted as white squares in the controls.

Another patch landed later on made the play/pause buttons white...