Bug 186424 - REGRESSION(macOS Mojave): move-by-word-visually-inline-block-positioned-element.html fails
Summary: REGRESSION(macOS Mojave): move-by-word-visually-inline-block-positioned-eleme...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-07 21:37 PDT by Ryosuke Niwa
Modified: 2018-06-08 13:37 PDT (History)
6 users (show)

See Also:


Attachments
WIP (1.25 KB, patch)
2018-06-07 21:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (4.47 KB, patch)
2018-06-07 23:45 PDT, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-06-07 21:37:44 PDT
The layout test fails on macOS Mojave.
Comment 1 Ryosuke Niwa 2018-06-07 21:38:11 PDT
Created attachment 342239 [details]
WIP
Comment 2 Ryosuke Niwa 2018-06-07 23:45:03 PDT
Created attachment 342244 [details]
Fixes the bug
Comment 3 Megan Gardner 2018-06-08 00:08:32 PDT
Wow, that’s one crazy bug! R=me, unofficially.
Comment 4 Ryosuke Niwa 2018-06-08 00:47:43 PDT
Comment on attachment 342244 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=342244&action=review

> Source/WebCore/ChangeLog:32
> +        wordBreakIteratorForMaxOffsetBoundary with visible position at offset 0 of "world", it thiss line and return nullptr.

It should read "it skip this line and return nullptr."
Comment 5 Alexey Proskuryakov 2018-06-08 09:04:18 PDT
Great work!

Is it possible to add a test that verifies this bug fix in a more straightforward manner, and maybe even extends testing in similar edge cases? Seems like we got lucky that a mostly unrelated test caught this.
Comment 6 Ryosuke Niwa 2018-06-08 13:06:04 PDT
(In reply to Alexey Proskuryakov from comment #5)
> Great work!
> 
> Is it possible to add a test that verifies this bug fix in a more
> straightforward manner, and maybe even extends testing in similar edge
> cases? Seems like we got lucky that a mostly unrelated test caught this.

I wouldn't say we got lucky since the failing test basically tests every word movement across divs with variety position values in order to catch exactly like the bug like this.

Having said that, I can add a new test which tests this exact failure although it would still require macOS Mojave to test it.

I'm not certain adding a new mostly redundant test like that is useful though...
Comment 7 Alexey Proskuryakov 2018-06-08 13:29:58 PDT
Sounds like it would not be very useful given that.
Comment 8 Ryosuke Niwa 2018-06-08 13:31:03 PDT
(In reply to Alexey Proskuryakov from comment #7)
> Sounds like it would not be very useful given that.

Okay. I'll land the patch as is then.
Comment 9 Ryosuke Niwa 2018-06-08 13:36:11 PDT
Committed r232635: <https://trac.webkit.org/changeset/232635>
Comment 10 Radar WebKit Bug Importer 2018-06-08 13:37:22 PDT
<rdar://problem/40948830>