Bug 229798

Summary: Use isRendererReplacedElement for SimplifiedBackwardsTextIterator
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: DOMAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, darin, ews-watchlist, mifenton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 229857    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch none

Description Frédéric Wang (:fredw) 2021-09-02 03:32:14 PDT
.
Comment 1 Frédéric Wang (:fredw) 2021-09-02 03:33:45 PDT
Created attachment 437132 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2021-09-02 07:03:37 PDT
Created attachment 437151 [details]
Patch
Comment 3 Frédéric Wang (:fredw) 2021-09-02 14:48:30 PDT
Comment on attachment 437151 [details]
Patch

Hi Chris, I think you reviewed this part of the code in the past. Can you please take a look?
Comment 4 Darin Adler 2021-09-02 15:21:49 PDT
Comment on attachment 437151 [details]
Patch

I assume the real benefits of this fix are more indirect, for the clients of SimplifiedBackwardsTextIterator, such as the visible boundary functions, which presumably give us incorrect results in these cases.

It’s unfortunate that we don’t really know what those cases are and so can only detect the improvement with a unit test of the iterator itself.
Comment 5 Frédéric Wang (:fredw) 2021-09-03 00:21:54 PDT
Comment on attachment 437151 [details]
Patch

It seems the crashes in mac-debug-wk1 are unrelated, the same happens for https://ews-build.webkit.org/#/builders/56/builds/14425

(In reply to Darin Adler from comment #4)
> Comment on attachment 437151 [details]
> Patch
> 
> I assume the real benefits of this fix are more indirect, for the clients of
> SimplifiedBackwardsTextIterator, such as the visible boundary functions,
> which presumably give us incorrect results in these cases.
>
> It’s unfortunate that we don’t really know what those cases are and so can
> only detect the improvement with a unit test of the iterator itself.

I agree. It seems the new cases were added for the sake of a11y. I wonder if our a11y thing is actually using SimplifiedBackwardsTextIterator. At least the visible boundary function is already emitting "," for some cases, so it looks like we want consistency here.

Also wonder if the isMedia() part is actually even used, we are no longer testing it at all after the move to modern media control. Will try to remove in a separate patch.
Comment 6 EWS 2021-09-03 00:25:40 PDT
Committed r281979 (241286@main): <https://commits.webkit.org/241286@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437151 [details].
Comment 7 Radar WebKit Bug Importer 2021-09-03 00:26:23 PDT
<rdar://problem/82712284>
Comment 8 Frédéric Wang (:fredw) 2021-09-03 08:44:27 PDT
(In reply to Frédéric Wang (:fredw) from comment #5)
> I agree. It seems the new cases were added for the sake of a11y. I wonder if
> our a11y thing is actually using SimplifiedBackwardsTextIterator. At least
> the visible boundary function is already emitting "," for some cases, so it
> looks like we want consistency here.
> 
> Also wonder if the isMedia() part is actually even used, we are no longer
> testing it at all after the move to modern media control. Will try to remove
> in a separate patch.

OK, I checked again and there are actually tests broken if we remove that. More at bug 229857. It seems the a11y code does not use SimplifiedBackwardsTextIterator, I was not able to trigger the code even with previousTextMarker. So I added another case in backwards-text-iterator-basic.html
Comment 9 Darin Adler 2021-09-03 13:52:40 PDT
(In reply to Frédéric Wang (:fredw) from comment #8)
> It seems the a11y code does not use
> SimplifiedBackwardsTextIterator

AXObjectCache::previousBoundary uses SimplifiedBackwardsTextIterator, and I believe that is accessibility code. Called by AXObjectCache::startCharacterOffsetOfSentence and AXObjectCache::startCharacterOffsetOfWord. Really surprised that accessibility has its own redundant copy of functions VisibleUnits.cpp; worth fixing that some day.
Comment 10 Frédéric Wang (:fredw) 2021-09-03 14:45:00 PDT
(In reply to Darin Adler from comment #9)
> (In reply to Frédéric Wang (:fredw) from comment #8)
> > It seems the a11y code does not use
> > SimplifiedBackwardsTextIterator
> 
> AXObjectCache::previousBoundary uses SimplifiedBackwardsTextIterator, and I
> believe that is accessibility code. Called by
> AXObjectCache::startCharacterOffsetOfSentence and
> AXObjectCache::startCharacterOffsetOfWord. Really surprised that
> accessibility has its own redundant copy of functions VisibleUnits.cpp;
> worth fixing that some day.

OK, thanks I hadn't tried to dig more into the code actually.  what I meant is that I did changes to text-marker-previous-next.html in https://bugs.webkit.org/attachment.cgi?id=437274&action=review which uses nextTextMarker/previousTextMarker ; I'm able to break that new test by tweaking TextIterator but not with SimplifiedBackwardsTextIterator.