| Summary: | Allow some image overlay content to render in fully transparent image elements | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
| Component: | Platform | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bdakin, cdumez, esprehn+autocc, ews-watchlist, kangil.han, megan_gardner, mifenton, thorton, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Wenson Hsieh
2021-03-25 21:53:54 PDT
Created attachment 424356 [details]
Patch
Comment on attachment 424356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424356&action=review > Source/WebCore/page/ImageOverlayController.cpp:86 > + if (overlayHostRenderer->style().opacity() > 0.01) > + return nullptr; This is sneakily hidden, and I almost missed it. Maybe possible to make it obvious that this is all inert in the "normal" case? Comment on attachment 424356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424356&action=review > Source/WebCore/dom/Document.cpp:2624 > if (auto* page = this->page()) { Can we merge with this block? > Source/WebCore/page/ImageOverlayController.cpp:54 > +ImageOverlayController::~ImageOverlayController() = default; Why do we need this destructor? > Source/WebCore/page/ImageOverlayController.h:46 > + ImageOverlayController(Page&); explicit? > Source/WebCore/page/ImageOverlayController.h:53 > + void willMoveToPage(PageOverlay&, Page*) override; final > Source/WebCore/page/ImageOverlayController.h:54 > + void didMoveToPage(PageOverlay&, Page*) override { } final > Source/WebCore/page/ImageOverlayController.h:55 > + void drawRect(PageOverlay&, GraphicsContext&, const IntRect& dirtyRect) override; final Comment on attachment 424356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424356&action=review >> Source/WebCore/dom/Document.cpp:2624 >> if (auto* page = this->page()) { > > Can we merge with this block? Yes — done! >> Source/WebCore/page/ImageOverlayController.cpp:54 >> +ImageOverlayController::~ImageOverlayController() = default; > > Why do we need this destructor? Whoops, we don't! Good catch. >> Source/WebCore/page/ImageOverlayController.cpp:86 >> + return nullptr; > > This is sneakily hidden, and I almost missed it. Maybe possible to make it obvious that this is all inert in the "normal" case? Yeah, that's a good point. I'm planning to split this bit out into a helper method, with a brief comment: ``` bool ImageOverlayController::shouldUsePageOverlayToPaintSelection(const RenderElement& renderer) { // If the selection is already painted (with nonzero opacity) in the overlay host's renderer, // then we don't need to fall back to a page overlay to paint the selection. return renderer.style().opacity() <= 0.01; } ``` >> Source/WebCore/page/ImageOverlayController.h:46 >> + ImageOverlayController(Page&); > > explicit? 👍🏻 >> Source/WebCore/page/ImageOverlayController.h:53 >> + void willMoveToPage(PageOverlay&, Page*) override; > > final 👍🏻 >> Source/WebCore/page/ImageOverlayController.h:54 >> + void didMoveToPage(PageOverlay&, Page*) override { } > > final 👍🏻 >> Source/WebCore/page/ImageOverlayController.h:55 >> + void drawRect(PageOverlay&, GraphicsContext&, const IntRect& dirtyRect) override; > > final 👍🏻 Created attachment 424393 [details]
Patch for landing
Committed r275112: <https://commits.webkit.org/r275112> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424393 [details]. |