Summary: | Remove unneeded webkit specific CSS attribute for Image Control Menu. | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||||||||||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, akeerthi, cdumez, changseok, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, joepeck, kangil.han, koivisto, kondapallykalyan, macpherson, menard, pdr, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan | ||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Megan Gardner
2021-12-16 13:54:45 PST
Created attachment 447391 [details]
Patch
Created attachment 447397 [details]
Patch
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? Created attachment 447402 [details]
Patch
Created attachment 447410 [details]
Patch
Created attachment 447414 [details]
Patch
Created attachment 447436 [details]
Patch
Patch with logging for debugging test output. 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;`. Created attachment 447656 [details]
Patch
Created attachment 447658 [details]
Patch
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 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? Created attachment 447827 [details]
Patch
Created attachment 447856 [details]
Patch
Created attachment 447862 [details]
Patch
Created attachment 447880 [details]
Patch
Created attachment 447926 [details]
Patch
Created attachment 448346 [details]
Patch
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. 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. Created attachment 448448 [details]
Patch for landing
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]. Reopening to attach new patch. Created attachment 448460 [details]
[fast-cq] Patch
|