<rdar://problem/30883525>
Created attachment 303731 [details] Patch
Comment on attachment 303731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303731&action=review TextIndicator parts are fine with me but we should get myles and smfr to take a peek at the other bits. > Source/WebCore/ChangeLog:58 > + Teach RenderReplaced to respect PaintBehaviorExcludeSelection by bailing from painting if it is selected and the > + paint behavior is enabled. What happens for <div> with background color (block /or/ inline-block)? One of the reasons I think we need smfr's input. > Source/WebCore/page/TextIndicator.h:52 > +enum TextIndicatorOption : uint16_t { Ahhhh this is when you know things have gotten bad.
Created attachment 303848 [details] Patch
Comment on attachment 303848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303848&action=review > Source/WebCore/page/TextIndicator.cpp:178 > + auto snapshotRect = enclosingIntRect(frame.view()->visualViewportRect()); Why visualViewportRect and not the snapshotRect? Does this mean that this snapshot is a different size than the others? That kind of breaks the model (e.g. you couldn't transition between the snapshots, which was the point of data.contentImageWithHighlight). > Source/WebCore/platform/graphics/FontCascade.cpp:352 > +float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned to, HashSet<const Font*>* fallbackFonts, float* outWidthBeforeRange, float* outWidthAfterRange) const Amazed that this is something we need to implement fresh. > Source/WebCore/rendering/InlineTextBox.cpp:567 > + if (!isHorizontal()) { What about vertical-bt :|
Comment on attachment 303848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303848&action=review >> Source/WebCore/page/TextIndicator.cpp:178 >> + auto snapshotRect = enclosingIntRect(frame.view()->visualViewportRect()); > > Why visualViewportRect and not the snapshotRect? Does this mean that this snapshot is a different size than the others? That kind of breaks the model (e.g. you couldn't transition between the snapshots, which was the point of data.contentImageWithHighlight). We talked about this in person -- we actually do need the full visible contents in this case, so let's rename TextIndicatorOptionIncludeSnapshotWithoutSelection to something more explicit, like TextIndicatorOptionIncludeSnapshotOfAllVisibleContentWithoutSelection >> Source/WebCore/platform/graphics/FontCascade.cpp:352 >> +float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned to, HashSet<const Font*>* fallbackFonts, float* outWidthBeforeRange, float* outWidthAfterRange) const > > Amazed that this is something we need to implement fresh. This is, indeed, the case. Previously, I took subruns of the whole text run and computed their widths which just used FontCascade::width, but Myles informed my that the Right Thing to do here is to use width iterators (either complex or simple, depending on the font) and advance to the appropriate location in the entire run. >> Source/WebCore/rendering/InlineTextBox.cpp:567 >> + if (!isHorizontal()) { > > What about vertical-bt :| Will add as a // FIXME
Created attachment 303872 [details] Patch for landing
Comment on attachment 303872 [details] Patch for landing Clearing flags on attachment: 303872 Committed r213614: <http://trac.webkit.org/changeset/213614>