Paint highlights specified in CSS Highlight API
Created attachment 385842 [details] Patch
Created attachment 385903 [details] Patch
Created attachment 385909 [details] Patch
Comment on attachment 385909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385909&action=review Now that you can actually test the highlight API's results, I think you need some more tests: 1. Replace a HighlightRangeGroup: CSS.highlights.set("example-highlight1", highlightRangeGroup1); // make another one CSS.highlights.set("example-highlight1", highlightRangeGroup2); 2. Test the ::highlight cascade: ::highlight(example-highlight1) {...} #foo::highlight(example-highlight1) {...} testing things inside and outside #foo, and with the same and different named highlights. > Source/WebCore/rendering/InlineTextBox.cpp:208 > +RenderObject::SelectionState InlineTextBox::rendererHighlightState(RenderObject* startObject, unsigned /*startPos*/, RenderObject* endObject, unsigned endPos) const Can the arguments be RenderObject&? > Source/WebCore/rendering/InlineTextBox.cpp:231 > + if (&renderer() == startObject || renderer().isDescendantOf(startObject)) { > + if (startObject && endObject && startObject == endObject) > + return RenderObject::SelectionBoth; > + if (startObject) > + return RenderObject::SelectionStart; > + } > + if (&renderer() == endObject || renderer().isDescendantOf(endObject)) > + return RenderObject::SelectionEnd; > + > + RenderObject* selectionEnd = nullptr; > + auto* selectionDataEnd = endObject; > + if (selectionDataEnd) > + selectionEnd = rendererAfterPosition(*selectionDataEnd, endPos); > + SelectionIterator selectionIterator(startObject); > + for (auto* currentRenderer = startObject; currentRenderer && currentRenderer != endObject; currentRenderer = selectionIterator.next()) { > + if (currentRenderer == startObject || currentRenderer == endObject) > + continue; > + if (!currentRenderer->canBeSelectionLeaf()) > + continue; > + if (&renderer() == currentRenderer) > + return RenderObject::SelectionInside; > + } Was this copied from somewhere, and can it be shared with selection code? > Source/WebCore/rendering/InlineTextBox.cpp:235 > +RenderObject::SelectionState InlineTextBox::highlightState(RenderObject* startObject, unsigned startPos, RenderObject* endObject, unsigned endPos) const Can the arguments be RenderObject&? > Source/WebCore/rendering/InlineTextBox.cpp:256 > + // The position after a hard line break is considered to be past its end. > + ASSERT(start() + len() >= (isLineBreak() ? 1 : 0)); > + unsigned lastSelectable = start() + len() - (isLineBreak() ? 1 : 0); > + > + bool start = (state != RenderObject::SelectionEnd && startPos >= m_start && startPos < m_start + m_len); > + bool end = (state != RenderObject::SelectionStart && endPos > m_start && endPos <= lastSelectable); > + if (start && end) > + state = RenderObject::SelectionBoth; > + else if (start) > + state = RenderObject::SelectionStart; > + else if (end) > + state = RenderObject::SelectionEnd; > + else if ((state == RenderObject::SelectionEnd || startPos < m_start) && (state == RenderObject::SelectionStart || endPos > lastSelectable)) > + state = RenderObject::SelectionInside; > + else if (state == RenderObject::SelectionBoth) > + state = RenderObject::SelectionNone; > + } Please share this code with the existing selection code. > Source/WebCore/rendering/InlineTextBox.cpp:632 > + if (!highlightMarkedTexts.isEmpty()) > + markedTexts.appendVector(WTFMove(highlightMarkedTexts)); Bad indent. > Source/WebCore/rendering/InlineTextBox.cpp:744 > +std::pair<unsigned, unsigned> InlineTextBox::highlightStartEnd(RenderObject* start, unsigned startPos, RenderObject* end, unsigned endPos) const Can the arguments be RenderObject&? > Source/WebCore/rendering/InlineTextBox.cpp:865 > + { Brace on previous line. > Source/WebCore/rendering/InlineTextBox.cpp:866 > + auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, markedText.highlightName}, &parent()->renderer().style()); Indent. > Source/WebCore/rendering/InlineTextBox.cpp:1099 > + Position endPos = createLegacyEditingPosition(staticRange->endContainer(), staticRange->endOffset()); Can we not use "legacy" things in new code? What's the replacement? > Source/WebCore/rendering/InlineTextBox.cpp:1103 > + int startOffset = startPos.deprecatedEditingOffset(); More deprecated. > Source/WebCore/rendering/InlineTextBox.h:124 > + std::pair<unsigned, unsigned> highlightStartEnd(RenderObject*, unsigned, RenderObject*, unsigned) const; The arguments should get names. > Source/WebCore/rendering/InlineTextBox.h:138 > + RenderObject::SelectionState rendererHighlightState(RenderObject* , unsigned , RenderObject* , unsigned) const; > + RenderObject::SelectionState highlightState(RenderObject*, unsigned, RenderObject*, unsigned) const; The names don't clarify how these are different. > Source/WebCore/rendering/MarkedText.cpp:83 > - result.append({ offsetSoFar, offsets[i].value, offsets[*frontmost].markedText->type, offsets[*frontmost].markedText->marker }); > + result.append({ offsetSoFar, offsets[i].value, offsets[*frontmost].markedText->type, offsets[*frontmost].markedText->marker, offsets[*frontmost].markedText->highlightName}); > } else { > // The appended marked texts may not be in paint order. We will fix this up at the end of this function. > for (unsigned j = 0; j < i; ++j) { > if (!processedMarkedTexts.contains(offsets[j].markedText)) > - result.append({ offsetSoFar, offsets[i].value, offsets[j].markedText->type, offsets[j].markedText->marker }); > + result.append({ offsetSoFar, offsets[i].value, offsets[j].markedText->type, offsets[j].markedText->marker, offsets[j].markedText->highlightName }); I don't really know what's going on here. If text has a combination of highlights and spelling underlines, do we need to subdivide based on both, or should we run subdivision separately for spelling underlines vs. highlights? > Source/WebCore/rendering/SelectionRangeData.h:43 > +class SelectionIterator { I think this needs a better name now that you exposed it. It's not iterating over the selection. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:6 > + <link rel="help" href="https://drafts.csswg.org/css-highlight-api-1/#creating-highlights"> This is not testing #creating-highlights, it's testing the painting I think? > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:8 > + <meta name="assert" content="Highlights should be able to be specified across elements."> Not quite sure what this means. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:30 > + <span id="text1">One two three</span> > + <span id="text2">four five six</span> > + <span id="text3">seven eight nine</span> > + <span id="text4">ten eleven twelve</span> > + <span id="text5">thirteen fourteen fifteen</span> At least one of these spans should not break on whitespace for better testing.
Comment on attachment 385909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385909&action=review > Source/WebCore/ChangeLog:11 > + Render highlights when present, similar to the way we render selection. This description should appear below "reviewed by" line but above the list of tests. > Source/WebCore/ChangeLog:1764 > + Move some helpful functions so they are accessible. > + What's up with this change? > Source/WebCore/rendering/InlineTextBox.cpp:591 > + if (RuntimeEnabledFeatures::sharedFeatures().highlightAPIEnabled()) { Instead of checking this at every caller, collectMarkedTextsForHighlights should have that check builtin. > Source/WebCore/rendering/InlineTextBox.cpp:1098 > + Position startPos = createLegacyEditingPosition(staticRange->startContainer(), staticRange->startOffset()); I don't think this works. See what FrameSelection::updateAppearance() does. We need to use the deepEquivalent of VisiblePosition, which is probably built at the end of each layout. Don't create these VisiblePosition here; that would be incorrect & cause security bugs. Instead, we ned to have some fixed time relative to updateLayout, perhaps post layout task, where we build up all VisiblePositions needed for each highlight group's ranges. >> Source/WebCore/rendering/InlineTextBox.cpp:1103 >> + int startOffset = startPos.deprecatedEditingOffset(); > > More deprecated. I don't think rendering code works with new non-deprecated node.
Comment on attachment 385909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385909&action=review > Source/WebCore/rendering/InlineTextBox.cpp:749 > + return { 0, 0}; Nit - Space between 0 and } >> Source/WebCore/rendering/InlineTextBox.cpp:866 >> + auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, markedText.highlightName}, &parent()->renderer().style()); > > Indent. Also, space between markedText.highlightName and } > Source/WebCore/rendering/InlineTextBox.cpp:888 > + if (decorations.contains(TextDecoration::Underline)) { > + style.textDecorationStyles.underlineColor = color; > + style.textDecorationStyles.underlineStyle = decorationStyle; > + } > + if (decorations.contains(TextDecoration::Overline)) { > + style.textDecorationStyles.overlineColor = color; > + style.textDecorationStyles.overlineStyle = decorationStyle; > + } > + if (decorations.contains(TextDecoration::LineThrough)) { > + style.textDecorationStyles.linethroughColor = color; > + style.textDecorationStyles.linethroughStyle = decorationStyle; > + } You could deduplicate logic in these if statements by using a single if statement, with decorations.containsAny(). > Source/WebCore/rendering/InlineTextBox.cpp:1110 > + markedTexts.append({ highlightStart, highlightEnd, MarkedText::Highlight, nullptr, highlightGroup.key}); Nit - Space between key and } > Source/WebCore/rendering/MarkedText.cpp:31 > +#include <wtf/text/WTFString.h> If the header includes WTFString.h, you shouldn't need this #include > Source/WebCore/rendering/MarkedText.cpp:78 > + result.append({ offsetSoFar, offsets[i].value, offsets[*frontmost].markedText->type, offsets[*frontmost].markedText->marker, offsets[*frontmost].markedText->highlightName}); Nit - space between highlightName and } > Source/WebCore/rendering/MarkedText.h:56 > + String highlightName { }; Nit - just String highlightName; is fine here. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements-expected.html:20 > +<body> > + <body> Multiple body tags. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:25 > +<body> > + <body> Multiple body tags. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-expected.html:20 > +<body> > + <body> Multiple body tags. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text.html:25 > +<body> > + <body> Multiple body tags.
Comment on attachment 385909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385909&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:1098 >> + Position startPos = createLegacyEditingPosition(staticRange->startContainer(), staticRange->startOffset()); > > I don't think this works. See what FrameSelection::updateAppearance() does. > We need to use the deepEquivalent of VisiblePosition, which is probably built at the end of each layout. > Don't create these VisiblePosition here; that would be incorrect & cause security bugs. > Instead, we ned to have some fixed time relative to updateLayout, perhaps post layout task, > where we build up all VisiblePositions needed for each highlight group's ranges. Are you envisioning some sort of parallel grouping of VisiblePositions? I'm not sure when you are thinking these should be built up. But this does work at least for simple websites/test cases. >> Source/WebCore/rendering/MarkedText.h:56 >> + String highlightName { }; > > Nit - just String highlightName; is fine here. you sure? I thought I needed a default constructor because this struct only has the implicit constructor, and if I want to make sure this is initialized, I thought I needed to do this explicitly? > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:9 > + <style> Just that they can start in one element and end in another. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:31 > + The test is really about the highlight ranges and them crossing each of these spans. I'm not sure what breaking the spans on non-whitespace would help test about highlights.
Comment on attachment 385909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385909&action=review >>> Source/WebCore/rendering/MarkedText.h:56 >>> + String highlightName { }; >> >> Nit - just String highlightName; is fine here. > > you sure? I thought I needed a default constructor because this struct only has the implicit constructor, and if I want to make sure this is initialized, I thought I needed to do this explicitly? I think Wenson is right. The implicit constructor should call the String's default constructor. type and endOffset not having default values above is fishy though.
Created attachment 386032 [details] Patch
I plan on renaming SelectionRangeData.cpp and SelectionRangeData.h, but anticipated some bikesheding around the name, so I am putting off the expensive file renames until PsudoElementRangeData is approved, or we come up with a better name.
(In reply to Megan Gardner from comment #9) > Created attachment 386032 [details] > Patch Ok, I'll pull it out then. I actually plan on make explicit constructors for this struct as it's gotten complicated enough that I think it would benefit from explicit constructors, but I'll do that in a follow up patch, as this is already getting out of control.
(In reply to Megan Gardner from comment #11) > (In reply to Megan Gardner from comment #9) > > Created attachment 386032 [details] > > Patch > > Ok, I'll pull it out then. I actually plan on make explicit constructors for > this struct as it's gotten complicated enough that I think it would benefit > from explicit constructors, but I'll do that in a follow up patch, as this > is already getting out of control. Tried to pull it out, it won't build, so I'll leave it in and deal with it when I make the explicit constructors.
Comment on attachment 386032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386032&action=review > Source/WebCore/rendering/InlineTextBox.cpp:178 > + updateEllipsisState(state); Is there really a point in separating this into its own helper function? It's only used once here. Why don't we just inline the code here instead? > Source/WebCore/rendering/InlineTextBox.cpp:182 > +RenderObject::SelectionState InlineTextBox::pseudoElementState(RenderObject::SelectionState state, PseudoElementRangeData& rangeData) const This is too generic of name. We're trying to compute the highlight state really so I'd call this highlightPseudoElementState instead. > Source/WebCore/rendering/InlineTextBox.cpp:1022 > + if (!RuntimeEnabledFeatures::sharedFeatures().highlightAPIEnabled()) { Nit: no curly braces around a single line statement. > Source/WebCore/rendering/InlineTextBox.cpp:1034 > + // <MMG> get peudoStyle here to save calculations Remove or use FIXME. > Source/WebCore/rendering/InlineTextBox.cpp:1047 > + Nit: whitespace. > Source/WebCore/rendering/InlineTextBox.cpp:1048 > + PseudoElementRangeData highlightData = PseudoElementRangeData(renderer().view()); Use auto? as in `auto highlightData = ~`. > Source/WebCore/rendering/InlineTextBox.cpp:1050 > + Nit: Whitespace. > Source/WebCore/rendering/SelectionRangeData.cpp:46 > +struct PseudoElementData { PseudoElementData is too generic of a name. I wouldn't call selection or highlight groups as a pseudo element. It usually refers to things like ::before & ::after: https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements I'd call it RangePseudoElementData or simply HighlightData instead since we can think of selection highlight as a type of highlight. > Source/WebCore/rendering/SelectionRangeData.cpp:162 > + // <MMG> these were pulled from elswhere in this file, refactor to make this work. Hopefully. Remove or use FIXME. > Source/WebCore/rendering/SelectionRangeData.h:43 > -class SelectionRangeData { > +class PseudoElementRangeData { Again, this is too generic of name. It should be probably something like HighlightRangeData. > ChangeLog:1 > +2019-12-18 Megan Gardner <megan_gardner@apple.com> Revert > WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:60 > - BuildableName = "libANGLE-shared.dylib" > + BuildableName = "libANGLE.a" Revert.
(In reply to Megan Gardner from comment #10) > I plan on renaming SelectionRangeData.cpp and SelectionRangeData.h, but > anticipated some bikesheding around the name, so I am putting off the > expensive file renames until PsudoElementRangeData is approved, or we come > up with a better name. I don't think PsudoElementRangeData is a good name. Pseudo element is way more than just selection or highlight groups. They refer to things like ::first-line, ::before, ::after, etc... https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements It would be too confusing to use PseudoElement prefix to refer to data structures related to selection & highlight groups.
Comment on attachment 386032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386032&action=review > Source/WebCore/rendering/InlineTextBox.cpp:577 > + auto highlightMarkedTexts = collectMarkedTextsForHighlights(TextPaintPhase::Foreground); > + if (!highlightMarkedTexts.isEmpty()) > + markedTexts.appendVector(WTFMove(highlightMarkedTexts)); Nit - this is indented by one extra level. > Source/WebCore/rendering/InlineTextBox.cpp:819 > + if (decorations.containsAny({ TextDecoration::Underline, TextDecoration::Overline, TextDecoration::Overline })) { This should be TextDecoration::Underline, TextDecoration::Overline, TextDecoration::LineThrough
Created attachment 386125 [details] Patch
Took out the rename, working on the leaks, but figured I could get some feedback in the meantime.
Comment on attachment 386125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386125&action=review Excited to see this coming together! > Source/WebCore/rendering/InlineTextBox.cpp:1030 > + for (auto& highlightGroup : renderer().document().highlightMap().map()) { You can make sue of destructing here. for (auto& [highlightKey, highlightGroup] : renderer().document().highlightMap().map()) { > Source/WebCore/rendering/InlineTextBox.cpp:1031 > + auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, highlightGroup.key }, &parent()->renderer().style()); Not sure if parent()->renderer()/parent()->renderer()->style() is expensive, but probably worth hoisting access outside the loop. > Source/WebCore/rendering/InlineTextBox.cpp:1035 > + Position startPos = createLegacyEditingPosition(staticRange->startContainer(), staticRange->startOffset()); > + Position endPos = createLegacyEditingPosition(staticRange->endContainer(), staticRange->endOffset()); Why is new code using LegacyEditingPositions? By the name of it, it seems like someone didn't want it to continued to be used. > Source/WebCore/rendering/InlineTextBox.cpp:1044 > + auto highlightData = SelectionRangeData(renderer().view()); Probably want to use constructor syntax rather than assignment. > Source/WebCore/rendering/InlineTextBox.cpp:1045 > + highlightData.setContext({startRenderer, endRenderer, static_cast<unsigned>(startOffset), static_cast<unsigned>(endOffset)}); Please add spaces after / before the braces. > Source/WebCore/rendering/InlineTextBox.h:138 > + RenderObject::SelectionState rendererHighlightState(RenderObject* , unsigned , RenderObject* , unsigned) const; > + RenderObject::SelectionState highlightState(RenderObject*, unsigned, RenderObject*, unsigned) const; Do these functions have implementations? They seem to be missing from the patch. > Source/WebCore/rendering/InlineTextBox.h:148 > + void updateEllipsisState(RenderObject::SelectionState); Do this function have an implementation? It seems to be missing from the patch.
(In reply to Sam Weinig from comment #18) > Comment on attachment 386125 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=386125&action=review > > > Source/WebCore/rendering/InlineTextBox.cpp:1035 > > + Position startPos = createLegacyEditingPosition(staticRange->startContainer(), staticRange->startOffset()); > > + Position endPos = createLegacyEditingPosition(staticRange->endContainer(), staticRange->endOffset()); > > Why is new code using LegacyEditingPositions? By the name of it, it seems > like someone didn't want it to continued to be used. Unfortunately, the rendering code hasn't been updated to handle non-legacy Position offsets. We should fix that but we decided it shouldn't be a blocker for this feature.
Created attachment 386157 [details] Patch
Created attachment 386168 [details] Patch
Created attachment 386171 [details] Patch
Comment on attachment 386171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386171&action=review > Source/WebCore/dom/Document.cpp:783 > + highlightMap().clear(); Nit - it looks like this will create m_highlightMap if it didn't already exist; we might want to consider something like this instead: if (m_highlightMap) m_highlightMap->clear();
Created attachment 386244 [details] Patch
Comment on attachment 386244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386244&action=review > Source/WebCore/rendering/InlineTextBox.cpp:162 > + // If there are ellipsis following, make sure their selection is updated. > + if (m_truncation != cNoTruncation && root().ellipsisBox()) { It's unclear why ellipsis won't affect CSS highlight groups. Did we test this? If not, can we file a bug somewhere to track it or add a FIXME? > Source/WebCore/rendering/InlineTextBox.cpp:804 > + case MarkedText::Highlight: > + { { should appear after : in the previous line like so: case MarkedText::Highlight: { > Source/WebCore/rendering/InlineTextBox.cpp:807 > + auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, markedText.highlightName }, &parent()->renderer().style()); > + > + if (renderStyle) { Declare renderStyle inside if as in: if (auto renderStyle = ~) > Source/WebCore/rendering/InlineTextBox.cpp:1021 > + Nit: whitespace. > Source/WebCore/rendering/InlineTextBox.cpp:1025 > + Ditto. > Source/WebCore/rendering/InlineTextBox.cpp:1034 > + if (renderStyle) { Continue when renderStyle is null instead to avoid deep nesting. > Source/WebCore/rendering/InlineTextBox.cpp:1045 > + Nit: whitespace. > Source/WebCore/rendering/InlineTextBox.cpp:1047 > + highlightData.setContext({startRenderer, endRenderer, static_cast<unsigned>(startOffset), static_cast<unsigned>(endOffset)}); Just FYI, there is no guarantee that offset is anything sane. It could be way higher than the actual number of characters or the children. Also, why don't we continue when either startRenderer or endRenderer is null here instead of checking it in setContext? > Source/WebCore/rendering/InlineTextBox.cpp:1048 > + Ditto. > Source/WebCore/rendering/InlineTextBox.h:138 > + Nit: whitespace. > Source/WebCore/rendering/SelectionRangeData.cpp:159 > + if ((context.start() && !context.end()) || (context.end() && !context.start())) Just do: !!context.start() != !!context.end(). But we should just get rid of this code entirely by checking this condition in the caller instead. Then we can assert that both start & end are set here. > Source/WebCore/rendering/SelectionRangeData.cpp:166 > + if (&renderer == m_selectionContext.start() || renderer.isDescendantOf(m_selectionContext.start())) { Why do we need this check? SelectionRangeData::apply doesn't do this. > Source/WebCore/rendering/SelectionRangeData.cpp:168 > + if (m_selectionContext.start() && m_selectionContext.end() && m_selectionContext.start() == m_selectionContext.end()) > + return RenderObject::SelectionBoth; How could this be correct? It would mean that every descendent of a render object which is both start & end would be marked as SelectionBoth. Perhaps you had to do this because you're not currently used canonicalized position out of VisiblePosition? This most certainly needs to be fixed in a follow up. I suggest adding FIXME here at least. > Source/WebCore/rendering/SelectionRangeData.cpp:170 > + if (m_selectionContext.start()) > + return RenderObject::SelectionStart; Ditto. > Source/WebCore/rendering/SelectionRangeData.cpp:173 > + if (&renderer == m_selectionContext.end() || renderer.isDescendantOf(m_selectionContext.end())) > + return RenderObject::SelectionEnd; Ditto.
Created attachment 386270 [details] Patch
Comment on attachment 386244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386244&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:162 >> + if (m_truncation != cNoTruncation && root().ellipsisBox()) { > > It's unclear why ellipsis won't affect CSS highlight groups. Did we test this? > If not, can we file a bug somewhere to track it or add a FIXME? They probably are affected, I didn't test this yet, but this code is in the 'wrong' place, so I'll file a radar to just clean it all up. >> Source/WebCore/rendering/SelectionRangeData.cpp:166 >> + if (&renderer == m_selectionContext.start() || renderer.isDescendantOf(m_selectionContext.start())) { > > Why do we need this check? SelectionRangeData::apply doesn't do this. It doesn't work if we don't check descendants. I expect this is a fallout of not using VisiblePositions yet, I will review when I make that fix. >> Source/WebCore/rendering/SelectionRangeData.cpp:168 >> + return RenderObject::SelectionBoth; > > How could this be correct? > It would mean that every descendent of a render object which is both start & end would be marked as SelectionBoth. > Perhaps you had to do this because you're not currently used canonicalized position out of VisiblePosition? > > This most certainly needs to be fixed in a follow up. I suggest adding FIXME here at least. Yes, I didn't read this one before I responded to the one above. I'll add a note about the radar I've already filed.
(In reply to Megan Gardner from comment #27) > Comment on attachment 386244 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=386244&action=review > > >> Source/WebCore/rendering/InlineTextBox.cpp:162 > >> + if (m_truncation != cNoTruncation && root().ellipsisBox()) { > > > > It's unclear why ellipsis won't affect CSS highlight groups. Did we test this? > > If not, can we file a bug somewhere to track it or add a FIXME? > > They probably are affected, I didn't test this yet, but this code is in the > 'wrong' place, so I'll file a radar to just clean it all up. Please also file a WebKit bugzilla bug & relate to this Bugzilla bug. > >> Source/WebCore/rendering/SelectionRangeData.cpp:168 > >> + return RenderObject::SelectionBoth; > > > > How could this be correct? > > It would mean that every descendent of a render object which is both start & end would be marked as SelectionBoth. > > Perhaps you had to do this because you're not currently used canonicalized position out of VisiblePosition? > > > > This most certainly needs to be fixed in a follow up. I suggest adding FIXME here at least. > > Yes, I didn't read this one before I responded to the one above. I'll add a > note about the radar I've already filed. Ditto.
Comment on attachment 386270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386270&action=review > Source/WebCore/dom/Document.cpp:784 > + if (m_highlightMap) > + m_highlightMap->clear(); Nice! > Source/WebCore/rendering/InlineTextBox.cpp:805 > + case MarkedText::Highlight: { > + if (auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, markedText.highlightName }, &parent()->renderer().style())) { Now that you've wrapped the whole thing, the outer { ~ } is unnecessary. > Source/WebCore/rendering/SelectionRangeData.cpp:165 > + // FIXME: we shouldln't have to check that a renderer is a descendant of the render node from the range. This is likely because we aren't using VisiblePositions yet. Planned fix in a followup: <rdar://problem/58095923> Please split this comment into two lines since it's really long. And also file a Bugzilla bug & prefer to that instead. People in the open source community won't be able to follow this otherwise. > LayoutTests/imported/w3c/ChangeLog:7 > + Please state from where thees tests are imported, and as of which Git hash.
Comment on attachment 386270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386270&action=review >> LayoutTests/imported/w3c/ChangeLog:7 >> + > > Please state from where thees tests are imported, and as of which Git hash. They aren't actually imported, I wrote them, with the intent of upstreaming them eventually, as there are no web platform tests yet.
Comment on attachment 386270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386270&action=review >>> LayoutTests/imported/w3c/ChangeLog:7 >>> + >> >> Please state from where thees tests are imported, and as of which Git hash. > > They aren't actually imported, I wrote them, with the intent of upstreaming them eventually, as there are no web platform tests yet. Okay, then they need to be under LayoutTests/http/wpt or just to LayoutTests/highlight. There is no need to put them in different directories. > LayoutTests/highlight/highlight-world-leak.html:5 > +<head> > +<script src="../resources/js-test-pre.js"></script> > +</head> Put this script element above <script> below instead of having it in head. > LayoutTests/highlight/highlight-world-leak.html:21 > + }, 10); Hm... this is dangerous approach. We have a conservative GC so it's possible that the document won't be collected. As I said previously in person, we should be creating ~20 framers and make sure at least a few of them e.g. 5 of them are collected instead. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-cascade.html:4 > + <meta charset="utf-8"> Please don't mix 2 space indentation with 4 space indentation. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-replace.html:4 > + <meta charset="utf-8"> Ditto about identation. > LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text.html:4 > + <meta charset="utf-8"> Ditto.
Created attachment 386279 [details] Patch for landing
Comment on attachment 386279 [details] Patch for landing Clearing flags on attachment: 386279 Committed r253857: <https://trac.webkit.org/changeset/253857>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58130458>
Hey Megan, sorry I'm late with this but there is one warning: [497/899] Building CXX object Source/W...ources/UnifiedSource-043dd90b-15.cpp.o In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-043dd90b-15.cpp:7: /home/mcatanzaro/Projects/WebKit/Source/WebCore/rendering/SelectionRangeData.cpp: In member function ‘WebCore::RenderObject::SelectionState WebCore::SelectionRangeData::selectionStateForRenderer(WebCore::RenderObject&)’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/rendering/SelectionRangeData.cpp:179:19: warning: variable ‘selectionEnd’ set but not used [-Wunused-but-set-variable] 179 | RenderObject* selectionEnd = nullptr; | ^~~~~~~~~~~~ I think the solution is: diff --git a/Source/WebCore/rendering/SelectionRangeData.cpp b/Source/WebCore/rendering/SelectionRangeData.cpp index d2188905703..ef82b36d5d4 100644 --- a/Source/WebCore/rendering/SelectionRangeData.cpp +++ b/Source/WebCore/rendering/SelectionRangeData.cpp @@ -176,10 +176,6 @@ RenderObject::SelectionState SelectionRangeData::selectionStateForRenderer(Rende if (&renderer == m_selectionContext.end()) return RenderObject::SelectionEnd; - RenderObject* selectionEnd = nullptr; - auto* selectionDataEnd = m_selectionContext.end(); - if (selectionDataEnd) - selectionEnd = rendererAfterOffset(*selectionDataEnd, m_selectionContext.endOffset().value()); SelectionIterator selectionIterator(m_selectionContext.start()); for (auto* currentRenderer = m_selectionContext.start(); currentRenderer && currentRenderer != m_selectionContext.end(); currentRenderer = selectionIterator.next()) { if (currentRenderer == m_selectionContext.start() || currentRenderer == m_selectionContext.end()) Do you agree? Or are there important side effects to rendererAfterOffset()? If so, we can cast the return value to (void) to suppress the warning.
No, I expect that is was an error as is supposed to be used in the next line, as that is what happens in the code that this was pulled from. Please do not commit that fix. I will look into this soon.