Bug 233305 – Re-add support of image control menus.

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Re-add support of image control menus.

[Megan Gardner]

Created attachment 444639 [details]
Patch

[Tim Horton]

Comment on attachment 444639 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferences.yaml:946
> +    WebKitLegacy:
> +      default: true
> +    WebKit:
> +      default: true
> +    WebCore:
> +      default: true

This seems surprising, why do we want it on by default?

> Source/WebCore/html/HTMLImageElement.cpp:784
> +void HTMLImageElement::tryCreateImageControls()

Lots of missing newlines between functions in this file

> Source/WebCore/html/shadow/ImageControlsRootElement.cpp:37
> +#endif

This file too. Did the place you copied this from crunch newlines? You might want to do a pass with your own eyes and fix

> Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:82
> +    button->setAttributeWithoutSynchronization(HTMLNames::classAttr, xWebkitImageControlsButtonName);

And again, all throughout. What happened?

> Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:85
> +    // FIXME: Why is right: 0px off the right edge of the parent?

What does this comment mean??

> Source/WebCore/page/EventHandler.cpp:2539
> +    if (candidateElement->hasEditableStyle())
> +        return nullptr;

This seems like a drive-by fix that maybe should be in its own patch? I could be wrong.

> Source/WebCore/rendering/RenderThemeMac.mm:97
> +// FIXME: This should go into an SPI.h file in the spi directory.

No reason not to do that right out of the gate.

> Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:162
> +#define WebKitImageControlsEnabledPreferenceKey @"WebKitImageControlsEnabled"

Let's not add back the webkitlegacy code

[Aditya Keerthi]

Comment on attachment 444639 [details]
Patch

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

> Source/WebCore/css/CSSValueKeywords.in:891
> +image-controls-button

I think we should call this `-webkit-image-controls-button` since it's not a standard keyword.

> Source/WebCore/html/shadow/mac/ImageControlsMac.css:37
> +    -webkit-appearance: image-controls-button;

Let's use `appearance: ...`.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:995
> +        "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", "image-controls-button", "-apple-pay-button", "textarea", "attachment", "borderless-attachment", "caps-lock-indicator",

No need for keyword completion when we make this keyword UA-sheet-only.

> LayoutTests/imported/w3c/web-platform-tests/css/css-ui/appearance-cssom-001-expected.txt:39
> +FAIL -webkit-appearance: image-controls-button (invalid) assert_equals: style.WebkitAppearance (uppercase W) expected "" but got "image-controls-button"

We should try to keep this WPT passing. To do this, we can't make `image-controls-button` a valid keyword for `appearance` / `-webkit-appearance` outside the UA stylesheet.

I think the easiest way to do this is to add a case to `isValueAllowedInMode` in CSSParserIdioms.cpp.

[Wenson Hsieh]

Comment on attachment 444639 [details]
Patch

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

> Source/WebCore/html/shadow/ImageControlsRootElement.h:30
> +class ImageControlsRootElement : public HTMLDivElement {

Same here, w.r.t. subclassing DOM elements.

> Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:43
> +class RenderImageControlsButton final : public RenderBlockFlow {

(Ditto.)

> Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.h:29
> +class ImageControlsButtonElementMac final : public HTMLDivElement {

(Ditto)

> Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:37
> +class RenderImageControls final : public RenderBlockFlow {

When I removed this code and replaced it with the current image overlay implementation, one of the main points of feedback I got was that I should avoid subclassing any renderers or HTML elements, and I ended up just using standard HTML elements (div, span, br, etc.) to build the image overlay subtree. I think we'd probably want to ensure this when restoring the original image controls code as well…

Perhaps Antti or Alan would have thoughts here?

[Wenson Hsieh]

> > Source/WebCore/page/EventHandler.cpp:2539
> > +    if (candidateElement->hasEditableStyle())
> > +        return nullptr;
> 
> This seems like a drive-by fix that maybe should be in its own patch? I
> could be wrong.

Yeah, I think this should probably be in a separate patch since it's kind of an independent behavior change.

I can address this in: https://bugs.webkit.org/show_bug.cgi?id=233317.

[zalan]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 444639 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444639&action=review
> 
> > Source/WebCore/html/shadow/ImageControlsRootElement.h:30
> > +class ImageControlsRootElement : public HTMLDivElement {
> 
> Same here, w.r.t. subclassing DOM elements.
> 
> > Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.cpp:43
> > +class RenderImageControlsButton final : public RenderBlockFlow {
> 
> (Ditto.)
> 
> > Source/WebCore/html/shadow/mac/ImageControlsButtonElementMac.h:29
> > +class ImageControlsButtonElementMac final : public HTMLDivElement {
> 
> (Ditto)
> 
> > Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:37
> > +class RenderImageControls final : public RenderBlockFlow {
> 
> When I removed this code and replaced it with the current image overlay
> implementation, one of the main points of feedback I got was that I should
> avoid subclassing any renderers or HTML elements, and I ended up just using
> standard HTML elements (div, span, br, etc.) to build the image overlay
> subtree. I think we'd probably want to ensure this when restoring the
> original image controls code as well…
> 
> Perhaps Antti or Alan would have thoughts here?
Good point! I think we should try not to introduce new renderers unless they drive the descendant tree layout in a non-standard way. These renderers do very little.

[Radar WebKit Bug Importer]

<rdar://problem/85742221>

[Megan Gardner]

Created attachment 445462 [details]
Patch

[Tim Horton]

Comment on attachment 445462 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/mac/NSServicesRolloverButtonCellSPI.h:40
> +#endif // ENABLE(SERVICE_CONTROLS)

You've got the endif comments backwards!

> Source/WebCore/dom/mac/ImageControlsMac.cpp:108
> +#endif // ENABLE(IMAGE_ANALYSIS)

I don't think this comment is right

> Source/WebCore/dom/mac/ImageControlsMac.h:43
> +#endif // ENABLE(IMAGE_ANALYSIS)

Or this one

> Source/WebCore/html/HTMLImageElement.cpp:77
> +    , m_imageMenuEnabled(true)

an odd choice of default. why true? also, probably put this initializer in the header (all of these could move... but not in this patch)

> Source/WebCore/html/HTMLImageElement.cpp:795
> +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(ImageControlsMac::hasControls((downcast<HTMLElement>(*node))));

extra parens!

> Source/WebCore/html/HTMLImageElement.h:119
> +    bool hasShadowControls() const { return m_imageMenuEnabled; }

why do we have this *and* hasImageControls()? is there a distinction here that isn't obvious? they seem to operate in quite different ways too

> Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.cpp:25
> +#include "config.h"

missing newline above

> Source/WebCore/html/shadow/mac/ImageControlsRootElementMac.h:25
> +#pragma once

This file is still all scrunched up.

> Source/WebCore/rendering/RenderThemeMac.mm:2294
> +}

all scrunched up

[Megan Gardner]

Created attachment 445473 [details]
Patch

[Megan Gardner]

Created attachment 445485 [details]
Patch

[Megan Gardner]

Created attachment 445490 [details]
Patch

[Megan Gardner]

Created attachment 445494 [details]
Patch

[Megan Gardner]

Created attachment 445496 [details]
Patch

[Megan Gardner]

Created attachment 445497 [details]
Patch

[Megan Gardner]

Created attachment 445500 [details]
Patch

[Megan Gardner]

Created attachment 445506 [details]
Patch

[Megan Gardner]

Created attachment 445523 [details]
Patch

[Megan Gardner]

Created attachment 445536 [details]
Patch

[Devin Rousso]

Comment on attachment 445536 [details]
Patch

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

> Source/WebCore/DerivedSources-input.xcfilelist:1322
> +$(PROJECT_DIR)/html/shadow/mac/imageControlsMac.css
>  $(PROJECT_DIR)/html/shadow/imageOverlay.css

Does this need to be alphabetical?

> Source/WebCore/PAL/pal/spi/mac/NSServicesRolloverButtonCellSPI.h:37
> +#define APPKIT_PRIVATE_CLASS

Do we need to `#undef` this?

> Source/WebCore/PAL/pal/spi/mac/NSServicesRolloverButtonCellSPI.h:44
> +@interface NSServicesRolloverButtonCell ()

Is it necessary for us to define this outside of `USE(APPLE_INTERNAL_SDK)`?  I'd imagine/hope that these symbols already (or will soon) exist in `AppKit/NSServicesRolloverButtonCell.h`.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:81
> +    if (RefPtr controlRoot = static_cast<TreeScope&>(*host->userAgentShadowRoot()).getElementById(imageControlsButtonIdentifier()))

Why do we need to `static_cast`?  `userAgentShadowRoot` returns a `ShadowRoot`, which inherits from `TreeScope`.
```
    if (RefPtr controlRoot = host->userAgentShadowRoot()->getElementById(imageControlsButtonIdentifier()))
```

> Source/WebCore/dom/mac/ImageControlsMac.h:35
> +namespace ImageControlsMac {

Is there a reason why this (and the file) has "Mac" in the name?  None of the code seems to require anything that only exists on macOS.

> Source/WebCore/html/HTMLImageElement.h:119
> +    bool imageMenuEnabled() const { return m_imageMenuEnabled; }

Should this also be inside `#if ENABLE(SERVICE_CONTROLS)`?

> Source/WebCore/html/HTMLImageElement.h:214
> +    bool m_imageMenuEnabled { false };

ditto (:119)

> Source/WebCore/html/shadow/mac/imageControlsMac.css:35
> +button#image-controls-button {

NIT: Should we make this
```
    div#image-controls button#image-controls-button
```
so that it's more specific and matches the selector below?

> Source/WebCore/page/EventHandler.cpp:2541
> +    if (candidateElement->hasEditableStyle())
> +        return nullptr;

This is a copy of the `if` right below.

> Source/WebCore/rendering/RenderImage.cpp:149
> +    if (is<HTMLImageElement>(element))
> +        m_hasShadowControls = downcast<HTMLImageElement>(element).imageMenuEnabled();

ditto (Source/WebCore/html/HTMLImageElement.h:119)

[Megan Gardner]

Comment on attachment 445536 [details]
Patch

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

>> Source/WebCore/DerivedSources-input.xcfilelist:1322
>>  $(PROJECT_DIR)/html/shadow/imageOverlay.css
> 
> Does this need to be alphabetical?

it is, I just put the sub directory before all the files in the directory, which seems to be not the way this file is alphabetized, but I swear others were when I looked.

>> Source/WebCore/dom/mac/ImageControlsMac.cpp:81
>> +    if (RefPtr controlRoot = static_cast<TreeScope&>(*host->userAgentShadowRoot()).getElementById(imageControlsButtonIdentifier()))
> 
> Why do we need to `static_cast`?  `userAgentShadowRoot` returns a `ShadowRoot`, which inherits from `TreeScope`.
> ```
>     if (RefPtr controlRoot = host->userAgentShadowRoot()->getElementById(imageControlsButtonIdentifier()))
> ```

And also from Document Fragment, so we need to specify.

>> Source/WebCore/dom/mac/ImageControlsMac.h:35
>> +namespace ImageControlsMac {
> 
> Is there a reason why this (and the file) has "Mac" in the name?  None of the code seems to require anything that only exists on macOS.

It's only for Mac, and we're using AppKit stuff to render the button. Also, this is how it was named before.

>> Source/WebCore/page/EventHandler.cpp:2541
>> +        return nullptr;
> 
> This is a copy of the `if` right below.

Oh, I think this was a bad merge when Wenson told me how to fix this, and then fixed it himself ;P

[Megan Gardner]

Created attachment 445653 [details]
Patch

[Megan Gardner]

Created attachment 445654 [details]
Patch

[Megan Gardner]

Created attachment 445655 [details]
Patch

[zalan]

Comment on attachment 445655 [details]
Patch

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

> Source/WebCore/html/shadow/mac/imageControlsMac.css:38
> +    top: 20px;
> +    right: 20px;

I assume these values are the direct translation of what imageControlsButtonPositionOffset() returned before, but it still feels odd to have such hard-coded top and right values as opposed to using some more flexible, alignment type of positioning.

> Source/WebCore/rendering/RenderTheme.h:256
> +#if ENABLE(SERVICE_CONTROLS)
> +    virtual IntSize imageControlsButtonSize() const { return IntSize(); }
> +    virtual IntSize imageControlsButtonPositionOffset() const { return IntSize(); }
> +#endif

I don't think these are needed anymore with the no-custom renderer approach.

[Megan Gardner]

Created attachment 445768 [details]
Patch

[Megan Gardner]

Created attachment 445770 [details]
Patch

[Megan Gardner]

Created attachment 445774 [details]
Patch

[Megan Gardner]

Created attachment 445776 [details]
Patch for landing

[EWS]

Committed r286461 (244802@main): <https://commits.webkit.org/244802@main>

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