Bug 234405 – Remove unneeded webkit specific CSS attribute for Image Control Menu.

• Component: New Bugs
• Status: REOPENED

[Megan Gardner]

Remove unneeded webkit specific CSS attribute for Image Control Menu.

[Megan Gardner]

Created attachment 447391 [details]
Patch

[Megan Gardner]

Created attachment 447397 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 447397 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        * UIProcess/API/Cocoa/WKWebViewConfiguration.mm:
> +        (-[WKWebViewConfiguration init]):

Did you actually change anything here?

> Source/WebCore/rendering/RenderTheme.cpp:307
> +#if ENABLE(SERVICE_CONTROLS)
> +    if (ImageControlsMac::isImageControlsButtonElement(element.get()))
> +        return ImageControlsButtonPart;
> +#endif

It's a bit weird to see macOS code in RenderTheme which has a RenderThemeMac subclass. Can we factor this into a virtual function?

[Megan Gardner]

Created attachment 447402 [details]
Patch

[Megan Gardner]

Created attachment 447410 [details]
Patch

[Megan Gardner]

Created attachment 447414 [details]
Patch

[Megan Gardner]

Created attachment 447436 [details]
Patch

[Megan Gardner]

Patch with logging for debugging test output.

[Aditya Keerthi]

Comment on attachment 447436 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeMac.mm:2317
> +bool RenderThemeMac::isImageControl(const Element* elementPtr) const

This should take a `const Element&` instead of a pointer, since you always dereference.

Can move the code you added to `RenderTheme::autoAppearanceForElement` below the `Ref element = *elementPtr;`.

[Megan Gardner]

Created attachment 447656 [details]
Patch

[Megan Gardner]

Created attachment 447658 [details]
Patch

[Aditya Keerthi]

Comment on attachment 447658 [details]
Patch

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

> Source/WebCore/css/CSSValueKeywords.in:-894
> --internal-image-controls-button

Because of the way we go from CSSValueID to ControlPart (see CSSPrimitiveValueMappings.h, `CSSPrimitiveValue::operator ControlPart()`), removing this value here also requires that the enum value `ImageControlsButtonPart` is moved to the end in the ControlPart enum.

Can you make the change in ThemeTypes.h, and add a comment like "// Internal-only values" to separate the value?

```
enum ControlPart {
    NoControlPart,
...
#if ENABLE(ATTACHMENT_ELEMENT)
    AttachmentPart,
    BorderlessAttachmentPart,
#endif
    CapsLockIndicatorPart,
    // Internal-only values
    ImageControlsButtonPart
};
```

> Source/WebCore/rendering/RenderThemeMac.mm:2317
> +bool RenderThemeMac::isImageControl(const Element* elementPtr) const

https://bugs.webkit.org/show_bug.cgi?id=234405#c9

[Wenson Hsieh]

Comment on attachment 447658 [details]
Patch

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

LGTM, with mine and Aditya's comments.

>> Source/WebCore/rendering/RenderThemeMac.mm:2317
>> +bool RenderThemeMac::isImageControl(const Element* elementPtr) const
> 
> https://bugs.webkit.org/show_bug.cgi?id=234405#c9

+1!

(Alternately — you could consider just using `ImageControlsMac::isImageControlsButtonElement` instead of adding this helper function on RenderThemeMac)

> LayoutTests/fast/images/image-controls-basic.html:32
> +	setInterval(() => {
> +    if (hasImageControls(elem))
> +        testRunner.notifyDone();
> +	}, 200);

Minor nit - it looks like this test mixes spaces and tabs; let's just use spaces?

[Megan Gardner]

Created attachment 447827 [details]
Patch

[Megan Gardner]

Created attachment 447856 [details]
Patch

[Megan Gardner]

Created attachment 447862 [details]
Patch

[Megan Gardner]

Created attachment 447880 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/86864533>

[Megan Gardner]

Created attachment 447926 [details]
Patch

[Megan Gardner]

Created attachment 448346 [details]
Patch

[Darin Adler]

Comment on attachment 448346 [details]
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:748
> +#endif // ENABLE(SERVICE_CONTROLS)

Should omit this comment here.

> Source/WebCore/html/shadow/mac/imageControlsMac.css:42
> -    appearance: -internal-image-controls-button;
> +    appearance: auto;

Should we remove this line entirely instead of changing it to say "auto"?

> Source/WebCore/platform/ThemeTypes.h:114
> +    LargestControlPart = ImageControlsButtonPart
> +#else
> +    LargestControlPart = CapsLockIndicatorPart

Can LargestControlPart be a constant outside the enumeration rather than an enumeration value?

    constexpr ControlPart largestControlPart = ImageControlsButtonPart;

It can otherwise be the same, still right here in the header next to the enumeration.

> Source/WebCore/rendering/RenderTheme.cpp:306
> +#endif
>      Ref element = *elementPtr;

I suggest a blank line here.

> Source/WebCore/rendering/RenderTheme.h:370
> +    virtual bool isImageControl(const Element*) const { return false; }

This should take a const Element&, because it dereferences the pointer without checking for nullptr.

> Source/WebCore/rendering/RenderThemeMac.mm:2321
> +    if (ImageControlsMac::isImageControlsButtonElement(*elementPtr))
> +        return true;
> +    return false;

Should just be return x, rather than if (x) return true; return false;

> Source/WebCore/rendering/style/RenderStyle.h:1143
> +        static_assert(LargestControlPart < 1 << APPEARANCE_BIT_WIDTH, "Control part must fit in storage bits");

I suggest we just put this static_assert outside the functions, alongside them. Since a static_assert works at runtime it does not need to be inside a function.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:69
> +#define APPEARANCE_BIT_WIDTH 7

This should be a constexpr integer, not a #define.

    constexpr int appearanceBitWidth = 7;

> Source/WebCore/testing/Internals.cpp:6397
> +#endif // ENABLE(SERVICE_CONTROLS)

This endif doesn’t need a comment, it’s so close to the start of the #If.

> Source/WebCore/testing/Internals.h:1194
> +#endif // ENABLE(SERVICE_CONTROLS)

This endif doesn’t need a comment, it’s so close to the start of the #If.

> LayoutTests/ChangeLog:11
> +        We need to move this test to be a mac specific one, as it now has mac specific 
> +        test harnessing. Also changed the test to be resistent to the async nature of adding
> +        the shadow dom elements, and checking the shadow dom directly rather than the resulting layout,
> +        which is prone to pixel errors.

What makes this test Mac-specific?

> LayoutTests/fast/images/mac/image-controls-basic.html:32
> +   		if (internals.shadowRoot(elem) && internals.shadowRoot(elem).getElementById('image-controls') && internals.shadowRoot(elem).getElementById('image-controls-button')) 
> +   	        output += 'PASS: image controls exist in shadowDom';
> +   		else
> +   			output += 'FAIL: no image controls found in shadowDom';
> +   		document.getElementById('log').innerHTML = output;

Broken indentation here.

> LayoutTests/fast/images/mac/image-controls-basic.html:40
> +    	checkForShadowDom(elem);

And here.

[Megan Gardner]

Comment on attachment 448346 [details]
Patch

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

>> Source/WebCore/html/shadow/mac/imageControlsMac.css:42
>> +    appearance: auto;
> 
> Should we remove this line entirely instead of changing it to say "auto"?

No, default appearance is 'none' so we need this line.

>> LayoutTests/ChangeLog:11
>> +        which is prone to pixel errors.
> 
> What makes this test Mac-specific?

hasImageControls - this is on Mac only, and since this test is written to wait until this returns true for async reasons the only way for the test to not time out would be for this to return true (which is blatantly not true), or for us to have a more complicated return system with a N/A return value, or... we could just run it on Mac, which the feature is Mac only anyways, so lets just run it on Mac.

[Megan Gardner]

Created attachment 448448 [details]
Patch for landing

[Megan Gardner]

rdar://86426074

[EWS]

Committed r287663 (245760@main): <https://commits.webkit.org/245760@main>

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

[Megan Gardner]

Reopening to attach new patch.

[Megan Gardner]

Created attachment 448460 [details]
[fast-cq] Patch

[Alex Christensen]

r287666