Bug 233305 - Re-add support of image control menus.
Summary: Re-add support of image control menus.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-17 22:10 PST by Megan Gardner
Modified: 2021-12-02 15:50 PST (History)
25 users (show)

See Also:


Attachments
Patch (83.71 KB, patch)
2021-11-17 22:16 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (73.72 KB, patch)
2021-11-30 13:18 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (65.20 KB, patch)
2021-11-30 14:58 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (65.21 KB, patch)
2021-11-30 16:23 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (65.29 KB, patch)
2021-11-30 17:04 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (65.80 KB, patch)
2021-11-30 17:32 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (64.96 KB, patch)
2021-11-30 17:49 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (61.23 KB, patch)
2021-11-30 18:03 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (60.67 KB, patch)
2021-11-30 18:24 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (60.67 KB, patch)
2021-11-30 20:06 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (61.55 KB, patch)
2021-11-30 22:14 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (61.55 KB, patch)
2021-12-01 00:20 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (60.85 KB, patch)
2021-12-01 18:42 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (61.09 KB, patch)
2021-12-01 18:53 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (61.09 KB, patch)
2021-12-01 19:00 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (60.47 KB, patch)
2021-12-02 13:11 PST, Megan Gardner
thorton: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (60.36 KB, patch)
2021-12-02 13:39 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (60.44 KB, patch)
2021-12-02 14:39 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (60.42 KB, patch)
2021-12-02 15:00 PST, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2021-11-17 22:10:01 PST
Re-add support of image control menus.
Comment 1 Megan Gardner 2021-11-17 22:16:46 PST
Created attachment 444639 [details]
Patch
Comment 2 Tim Horton 2021-11-17 22:34:21 PST
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
Comment 3 Aditya Keerthi 2021-11-17 23:29:04 PST
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.
Comment 4 Wenson Hsieh 2021-11-18 08:00:35 PST
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?
Comment 5 Wenson Hsieh 2021-11-18 08:04:18 PST
> > 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.
Comment 6 zalan 2021-11-18 08:51:48 PST
(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.
Comment 7 Radar WebKit Bug Importer 2021-11-24 22:10:19 PST
<rdar://problem/85742221>
Comment 8 Megan Gardner 2021-11-30 13:18:37 PST
Created attachment 445462 [details]
Patch
Comment 9 Tim Horton 2021-11-30 13:29:01 PST
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
Comment 10 Megan Gardner 2021-11-30 14:58:31 PST
Created attachment 445473 [details]
Patch
Comment 11 Megan Gardner 2021-11-30 16:23:23 PST
Created attachment 445485 [details]
Patch
Comment 12 Megan Gardner 2021-11-30 17:04:00 PST
Created attachment 445490 [details]
Patch
Comment 13 Megan Gardner 2021-11-30 17:32:26 PST
Created attachment 445494 [details]
Patch
Comment 14 Megan Gardner 2021-11-30 17:49:35 PST
Created attachment 445496 [details]
Patch
Comment 15 Megan Gardner 2021-11-30 18:03:55 PST
Created attachment 445497 [details]
Patch
Comment 16 Megan Gardner 2021-11-30 18:24:56 PST
Created attachment 445500 [details]
Patch
Comment 17 Megan Gardner 2021-11-30 20:06:49 PST
Created attachment 445506 [details]
Patch
Comment 18 Megan Gardner 2021-11-30 22:14:07 PST
Created attachment 445523 [details]
Patch
Comment 19 Megan Gardner 2021-12-01 00:20:00 PST
Created attachment 445536 [details]
Patch
Comment 20 Devin Rousso 2021-12-01 13:30:31 PST
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)
Comment 21 Megan Gardner 2021-12-01 18:35:25 PST
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
Comment 22 Megan Gardner 2021-12-01 18:42:42 PST
Created attachment 445653 [details]
Patch
Comment 23 Megan Gardner 2021-12-01 18:53:04 PST
Created attachment 445654 [details]
Patch
Comment 24 Megan Gardner 2021-12-01 19:00:47 PST
Created attachment 445655 [details]
Patch
Comment 25 zalan 2021-12-02 10:54:00 PST
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.
Comment 26 Megan Gardner 2021-12-02 13:11:48 PST
Created attachment 445768 [details]
Patch
Comment 27 Megan Gardner 2021-12-02 13:39:07 PST
Created attachment 445770 [details]
Patch
Comment 28 Megan Gardner 2021-12-02 14:39:44 PST
Created attachment 445774 [details]
Patch
Comment 29 Megan Gardner 2021-12-02 15:00:12 PST
Created attachment 445776 [details]
Patch for landing
Comment 30 EWS 2021-12-02 15:50:05 PST
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].