Repaint as needed when adding and removing highlights
Created attachment 410084 [details] Patch
<rdar://problem/59076190>
Comment on attachment 410084 [details] Patch Why do we need both this repaint code, and HighlightData::repaint()?
Because HighlightData(s) are ephemeral and only used for/while rendering in relation to CSS Highlights. They don't exist when ranges are added or removed from HighlightRangeGroup, and there isn't a direct relationship between the objects. So when we add or remove a new range, we need to mark that node as needing a repaint. Also, HighlightData::Repaint to be used specifically when HighlightData is being used for selections, which are stored an handled very differently than CSS Highlights. HighlightData used to be called SelectionData, but when I started to overload its usage for CSS Highlights, I changed the name. There is probably still work to be done to clean up this class and have the names be more reflective of their current use.
Created attachment 410151 [details] Patch
Comment on attachment 410151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410151&action=review > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:84 > + repaintRange(range); Can we avoid these repaints if the HighlightRangeGroup has no associated styles?
Comment on attachment 410151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410151&action=review >> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:84 >> + repaintRange(range); > > Can we avoid these repaints if the HighlightRangeGroup has no associated styles? Not the way the data is structured currently, but when I change to match the spec (where the name is stored along side the ranges), then it will be possible, I'll file a bug to track the work.
Created attachment 410173 [details] Patch
Comment on attachment 410173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410173&action=review > Source/WebCore/ChangeLog:9 > + When adding and removing psudo highlights, make sure that the A typo? s/psudo/pseudo/ > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:91 > + for (Ref<HighlightRangeData>& data : m_rangesData) Can we use auto here? > LayoutTests/http/wpt/css/css-highlight-api/highlight-text-repaint.html:62 > + <script> Can be removed?
Created attachment 410257 [details] Patch
Created attachment 410290 [details] Patch for landing
Committed r267863: <https://trac.webkit.org/changeset/267863> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410290 [details].
Comment on attachment 410290 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=410290&action=review Some follow-up ideas. > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:58 > +static void repaintRange(const StaticRange& range) Could change this to SimpleRange. > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:71 > + if (is_gt(ordering)) { > + startNode = &range.endContainer(); > + endNode = &range.startContainer(); > + } Could do this: if (is_gt(ordering)) std::swap(startNode, endNode); Unclear why we are handling the case where they end up out of order, but not the case where one end or the other isn’t in the document any more, which can also happen. If we can prevent that from happening by updating the ranges as the DOM is mutated, then we can also prevent this out-of-order case from happening at that time as well. > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:74 > + auto node = startNode; > + while (node != endNode) { Can clean this up to use intersectingNodes. Likely would not need the is_eq check above at all if we did that, since I think it does the right thing.