Summary: | Native text selection UI is incorrectly suppressed in Microsoft Visio | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
Component: | HTML Editing | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bdakin, bfulgham, commit-queue, darin, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Wenson Hsieh
2019-02-28 11:53:04 PST
Created attachment 363401 [details]
WIP
I'm gonna leave this one for zalan/smfr Comment on attachment 363401 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=363401&action=review > Source/WebCore/rendering/RenderObject.cpp:1558 > + auto* container = &enclosingBox(); > + while (container) { Nicer to write as a for loop. If we can’t make the third clause work we can still write: for (auto* container = &enclosingBox(); container; ) { > Source/WebCore/rendering/RenderObject.cpp:1566 > + auto& frame = container->frame(); I think this logic about walking out to the enclosing frame's renderer should be put in a helper function. Writing it out as a sequence of if statements makes the thing look too complicated and it’s really a single concept. Not sure exactly which helper function to write. Here’s one: static RenderBox* enclosingFrameRenderBox(RenderBox& container) { auto* owner = container.frame().ownerElement(); if (!owner) return nullptr; auto* renderer = owner->renderer(); return is<RenderBox>(renderer) ? &downcast<RenderBox>(*renderer) : nullptr; } Then: static RenderBox* containingBlockCrossingFrameBoundaries(RenderBox& container) { auto* nextContainer = container.containingBlock(); if (!nextContainer) nextContainer = enclosingFrameRenderBox(container); return nextContainer ? nextContainer : enclosingFrameRenderBox(container); } Then: for (auto* container = &enclosingBox(); container; container = containingBlockCrossingFrameBoundaries(*container)) But I think there are other interesting factorings too that might allow sharing even more code with other call sites. > Source/WebCore/rendering/RenderObject.cpp:1572 > + if (frame.isMainFrame()) > + break; > + > + auto* owner = frame.ownerElement(); > + if (!owner) > + break; It’s unnecessary to check both isMainFrame and check ownerElement for null. I suggest skipping the isMainFrame check. > Source/WebCore/rendering/RenderObject.cpp:1576 > + if (!frameRenderer || !frameRenderer->isBox()) > + break; if (!is<RenderBox>(frameRenderer)) break; Comment on attachment 363401 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=363401&action=review > Source/WebCore/ChangeLog:9 > + Currently, our heuristics for detecting hidden editable areas attempts to search for empty parent renderers with heuristics...attempts -> heuristics...attempt > Source/WebCore/rendering/RenderObject.cpp:1563 > + bool isOverflowHidden = style.overflowX() == Overflow::Hidden || style.overflowY() == Overflow::Hidden; > + if (isOverflowHidden && !container->isDocumentElementRenderer() && !container->isBody() && container->contentSize().isEmpty()) > + return true; > + I think this code needs to be rewritten in terms of RenderLayer::clipRectRelativeToAncestor() or something similar. These are questions you ask of the RenderLayer rather than trying to compute yourself, which is fraught with danger. Comment on attachment 363401 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=363401&action=review >> Source/WebCore/ChangeLog:9 >> + Currently, our heuristics for detecting hidden editable areas attempts to search for empty parent renderers with > > heuristics...attempts -> heuristics...attempt Good catch! >> Source/WebCore/rendering/RenderObject.cpp:1558 >> + while (container) { > > Nicer to write as a for loop. If we can’t make the third clause work we can still write: > > for (auto* container = &enclosingBox(); container; ) { Sounds good — I rewrote both this and the while loop above as for loops, using static helpers to advance each iteration. >> Source/WebCore/rendering/RenderObject.cpp:1563 >> + > > I think this code needs to be rewritten in terms of RenderLayer::clipRectRelativeToAncestor() or something similar. These are questions you ask of the RenderLayer rather than trying to compute yourself, which is fraught with danger. It wasn't clear how to make use of this; one approach that seems to work reasonably well is to check whether the enclosing layer's selfClipRect is empty, but this doesn't account for the scenario in which the enclosing layer's clippingRootForPainting() is itself (for instance, when a text field has "transform: translateZ(0);"). I was able to fix this by introducing a new helper on RenderLayer to the effect of hasEmptyClipRectRelativeToDocumentView() that computes the clip rect relative to the document view's layer instead of the clipping root for painting. But this affects no known sites and I'm not sure it's worth the additional code on RenderLayer, so I omitted it in the new patch (forthcoming). >> Source/WebCore/rendering/RenderObject.cpp:1566 >> + auto& frame = container->frame(); > > I think this logic about walking out to the enclosing frame's renderer should be put in a helper function. Writing it out as a sequence of if statements makes the thing look too complicated and it’s really a single concept. Not sure exactly which helper function to write. Here’s one: > > static RenderBox* enclosingFrameRenderBox(RenderBox& container) > { > auto* owner = container.frame().ownerElement(); > if (!owner) > return nullptr; > auto* renderer = owner->renderer(); > return is<RenderBox>(renderer) ? &downcast<RenderBox>(*renderer) : nullptr; > } > > Then: > > static RenderBox* containingBlockCrossingFrameBoundaries(RenderBox& container) > { > auto* nextContainer = container.containingBlock(); > if (!nextContainer) > nextContainer = enclosingFrameRenderBox(container); > return nextContainer ? nextContainer : enclosingFrameRenderBox(container); > } > > Then: > > for (auto* container = &enclosingBox(); container; container = containingBlockCrossingFrameBoundaries(*container)) > > But I think there are other interesting factorings too that might allow sharing even more code with other call sites. Ok! I pulled out some common logic into static helpers, and used these to rewrite the two while loops here as for loops. >> Source/WebCore/rendering/RenderObject.cpp:1572 >> + break; > > It’s unnecessary to check both isMainFrame and check ownerElement for null. I suggest skipping the isMainFrame check. Fixed! >> Source/WebCore/rendering/RenderObject.cpp:1576 >> + break; > > if (!is<RenderBox>(frameRenderer)) > break; I ended up removing this part. Created attachment 363518 [details]
Take two
Comment on attachment 363518 [details]
Take two
Wow, so much better in both algorithm and coding style too!
Comment on attachment 363518 [details]
Take two
Thanks for the review!
Comment on attachment 363518 [details] Take two Clearing flags on attachment: 363518 Committed r242401: <https://trac.webkit.org/changeset/242401> All reviewed patches have been landed. Closing bug. |