| Summary: | Incorrect text caret placement when RTL text starts with whitespace | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ebrahim Byagowi <ebrahim> | ||||||||||||||
| Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | bfulgham, koivisto, mmaxfield, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | Safari Technology Preview | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| URL: | data:text/html;charset=utf8,<textarea%20dir=rtl%20style="font-size:%20500%">%20شسی%20شسی%20شسی%20شسی%20شسی%20شسی%20شسی%20شسی</textarea><script>document.querySelector('textarea').focus()</script> | ||||||||||||||||
| Bug Depends on: | 251414 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 464698 [details]
Caret
This is about incorrect hanging whitespace handling in RTL context. When the textarea's style is changed from "white-space: pre-wrap" to 'white-space: pre", it starts working fine. Looking at the individual text run positions, the logically leading (visually trailing) whitespace on the first line is at -5px (this is logical x -5px, which essentially means whitespace is put outside the textarea). Created attachment 464742 [details]
Patch
> Patch
Just tested the patch and works as expected, awesome. Thanks 😊
(In reply to Ebrahim Byagowi from comment #5) > > Patch > > Just tested the patch and works as expected, awesome. Thanks 😊 Thank you so much! I decided to go with a more (RTL) focused approach though -will post a new patch soon (sorry about that, will use this patch as a more generic fix for pre-wrap vs. scrollable overflow under a different bug). Created attachment 464751 [details]
Patch
Tested the new patch also and it works great, this use of C++ lambdas to contain variables needed to calculate one value is an interesting pattern I found in these fixes, hopefully I will use it somewhere I had access to a C++ codebase, and, thanks again 😊 (In reply to Ebrahim Byagowi from comment #8) > Tested the new patch also and it works great, this use of C++ lambdas to > contain variables needed to calculate one value is an interesting pattern I > found in these fixes, hopefully I will use it somewhere I had access to a > C++ codebase, and, thanks again 😊 I find it useful when I need to scope things for clarify (especially with this many lines of comment) but don't want to introduce a static helper function. Created attachment 464805 [details]
Patch
Is that underline behind hanging space on svg/compositing/outermost-svg-with-border-padding.html worrisome? Apparently other browsers do that also,
data:text/html;charset=utf8,<u%20dir=rtl%20style="width:700px;display:block;font-size:%20500%;%20white-space:%20pre-wrap">%20%D8%B4%D8%B3%DB%8C%20%D8%B4%D8%B3%DB%8C%20%D8%B4%D8%B3%DB%8C%20%D8%B4%D8%B3%DB%8C%20%D8%B4%D8%B3%DB%8C%20%D8%B4%D8%B3%DB%8C%20%D8%B4%D8%B3%DB%8C%20%D8%B4%D8%B3%DB%8C</textarea><script>document.querySelector('textarea').focus()</u>
> Apparently other browsers do that also,
But it shouldn't happen beyond the element borders apparently, hmmm.
(In reply to Ebrahim Byagowi from comment #12) > > Apparently other browsers do that also, > > But it shouldn't happen beyond the element borders apparently, hmmm. Chrome overhangs that trailing space too (see attached screenshot). going to take a look at that failing test case and land this patch shortly. Created attachment 464830 [details]
overhanging trailing space
(this is with overflow: visible. the failing test has overflow auto/scroll. This is about whether the trailing hanging whitespace should contribute to scrollable or ink overflow -current behavior is ink, but with this patch we start contributing to scrollable overflow) Created attachment 464842 [details]
[fast-cq]Patch
> Patch
Applied the patch on top of tip of tree (6c4c9810) and things look great, thanks 😊
(In reply to Ebrahim Byagowi from comment #17) > > Patch > > Applied the patch on top of tip of tree (6c4c9810) and things look great, > thanks 😊 Thank you! Very much appreciated! Committed 259868@main (f500d3412d93): <https://commits.webkit.org/259868@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 464842 [details]. |
This is a reduction of something that hits me on RTL websites and happens both in Safari 16.2 and tip of tree (5dc808f2): Steps to reproduce: Open data:text/html;charset=utf8,<textarea%20dir=rtl%20style="font-size:%20500%">%20شسی%20شسی%20شسی%20شسی%20شسی%20شسی%20شسی%20شسی</textarea><script>document.querySelector('textarea').focus()</script> (if caret didn't appear, click on the textarea, press Command+A, press the right arrow key button) Expected: The caret should go to right most position, start of the text in RTL. Actual: The caret goes after the first space, as can be seen in the attached screen cast.