Bug 182181

Summary: Range getBoundingClientRect returning zero rect on simple text node with <br> before it
Product: WebKit Reporter: Benjamin San Souci <benjamin.sansouci>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, dbates, giosia, koivisto, rniwa, simon.fraser, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: Safari 11   
Hardware: Unspecified   
OS: macOS 10.13   
Attachments:
Description Flags
Fix Range end offsets
none
Fix Range end offsets
none
Fix Range end offsets none

Benjamin San Souci
Reported 2018-01-26 12:42:52 PST
Safari version: 11.0.2 Here's the html to repro this: ``` <html> <head> </head> <body> <div> Something goes here! <br> Now more goes here. </div> <script> let r = new Range(); let node = document.getElementsByTagName('div')[0].childNodes[2]; r.setStart(node, 0); r.setEnd(node, 15); let rect = r.getBoundingClientRect(); alert(JSON.stringify(rect)); </script> </body> </html> ``` I get a 0,0,0,0 rect. I can repro 100% of the time on one laptop, but can't at all on another until I turn on "Responsive Design Mode" and then I can repro for that tab again. This happens reliably on 3 different iPhone with iOS 11.2. Does not happen on chrome or firefox. I also can't find document that would explain why this is happening. If this is expected, then would you have a suggestion for how to get a rectangle around part of that 2nd text node? This only happens when there's a <br> followed by a text node. The only maybe related bug I've found was https://bugs.webkit.org/show_bug.cgi?id=38394, but that one has a more complicated repro procedure. I don't know the Webkit codebase at all, but my guess is that this function: https://github.com/WebKit/webkit/blob/master/Source/WebCore/rendering/SimpleLineLayoutResolver.cpp#L205 returns an empty range, making the body of this loop https://github.com/WebKit/webkit/blob/7bdbecb7e435a102c10bc4ed27986720ae84ce95/Source/WebCore/rendering/SimpleLineLayoutFunctions.cpp#L233 never execute, and so the whole thing returns an empty collection of quads. The reason is simply that it seems like all codepaths inside the for loop add something to the vector of quads. Just a thought.
Attachments
Fix Range end offsets (6.68 KB, patch)
2019-03-06 20:30 PST, Gabe Giosia 		no flags
DetailsFormatted DiffDiff
Fix Range end offsets (6.20 KB, patch)
2019-03-06 20:55 PST, Gabe Giosia 		no flags
DetailsFormatted DiffDiff
Fix Range end offsets (6.41 KB, patch)
2019-03-07 02:14 PST, Gabe Giosia 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Benjamin San Souci
Comment 1 2018-02-01 22:53:22 PST
I've been able to repro this with 2 text nodes next to each other under a div: ``` <html> <head> </head> <body> <script> let d = document.createElement('div'); d.appendChild(document.createTextNode("Testing something")); d.appendChild(document.createTextNode("And another thing here")); document.body.appendChild(d); let r = new Range(); let node = document.getElementsByTagName('div')[0].childNodes[1]; r.setStart(node, 0); r.setEnd(node, 15); let rect = r.getBoundingClientRect(); alert(JSON.stringify(rect)); </script> </body> </html> ``` This will popup a 0,0,0,0 rect.
Benjamin San Souci
Comment 2 2018-02-01 23:06:27 PST
Here is a more illustrative example that might help. I highlight 3 ranges - one from the first text node's to the 2nd text node's 15th char - one from the first text node's to the 3rd text node's 0th char - one from the first text node's to the 3rd text node's 15th char The first range goes to the 0th char of the 2nd text node instead of the 15th char. The last two ranges return the same bounding rect. ``` <html> <head> </head> <body> <script> function drawRect(rect) { let d = document.createElement("div"); d.style.backgroundColor = "red"; d.style.left = rect.left + "px"; d.style.top = rect.top + "px"; d.style.width = rect.width + "px"; d.style.height = rect.height + "px"; d.style.position = "absolute"; d.style.opacity = 0.5; document.body.appendChild(d); } let d = document.createElement('div'); d.appendChild(document.createTextNode("Testing something")); d.appendChild(document.createTextNode("Testing and things here")); d.appendChild(document.createTextNode("And another thing here")); document.body.appendChild(d); { let r = new Range(); r.setStart(document.getElementsByTagName('div')[0].childNodes[0], 0); r.setEnd(document.getElementsByTagName('div')[0].childNodes[1], 15); drawRect(r.getBoundingClientRect()); } { let r = new Range(); r.setStart(document.getElementsByTagName('div')[0].childNodes[0], 0); r.setEnd(document.getElementsByTagName('div')[0].childNodes[2], 0); drawRect(r.getBoundingClientRect()); } { let r = new Range(); r.setStart(document.getElementsByTagName('div')[0].childNodes[0], 0); r.setEnd(document.getElementsByTagName('div')[0].childNodes[2], 15); drawRect(r.getBoundingClientRect()); } </script> </body> </html> ```
Gabe Giosia
Comment 3 2019-03-06 20:24:22 PST
*** Bug 192495 has been marked as a duplicate of this bug. ***
Gabe Giosia
Comment 4 2019-03-06 20:30:27 PST
Created attachment 363836 [details] Fix Range end offsets This patch changes the way the end offsets are calculated. It (correctly) uses the local-end offsets rather than using the end-offset from within the node. The existing code was mostly working before because contained nodes use maxInt to signify fully contained so this could only effect the last node.
Gabe Giosia
Comment 5 2019-03-06 20:55:09 PST
Created attachment 363840 [details] Fix Range end offsets Updated patch to remove some changes that didn't need to be made.
Daniel Bates
Comment 6 2019-03-06 23:01:53 PST
Comment on attachment 363840 [details] Fix Range end offsets View in context: https://bugs.webkit.org/attachment.cgi?id=363840&action=review > LayoutTests/ChangeLog:3 > + Test getBoundingClientRect with a Range that contains a line break. Not conformant to our style. This line should be bug title verbatim. Description goes under reviewed by. > Source/WebCore/ChangeLog:3 > + Correct height of RenderText in a block element when used in a Range Ditto.
Gabe Giosia
Comment 7 2019-03-07 02:14:01 PST
Created attachment 363861 [details] Fix Range end offsets updates patch Changelog
Gabe Giosia
Comment 8 2019-03-08 07:32:35 PST
I first started seeing this bug sometime around iOS11; It seems to have been introduced in https://trac.webkit.org/changeset/212693/webkit I have worked around the bug by wrapping text in a span-tag when using Range.getBoundingClientRect(). Starting a Range in an in-line-level element live a span(rather than block-element like a div) bypasses the buggy code by disabling the use of simpleLineLayout. Using this span-wrap-hack has many side-effects and isn't ideal(dom modification, CSS selector breakage, etc). It's likely possible that some of the simpleLineLayout bypass-code can also be removed. e.g. > if (simpleLineLayout() && parent()->firstChild() == parent()->lastChild()) {
Gabe Giosia
Comment 9 2019-03-08 07:43:55 PST
> if (simpleLineLayout() && parent()->firstChild() == parent()->lastChild()) { comes from https://trac.webkit.org/changeset/215054/webkit which appears to have been a workaround for this same bug. I'm not sure if it's best to revert that in the same patch as fixing the Range bug, or would it be better to do that separately?
Ryosuke Niwa
Comment 10 2019-03-11 15:08:02 PDT
This needs to be reviewed by Simon or Zalan.
Gabe Giosia
Comment 11 2019-03-19 07:24:48 PDT
@zalan@apple.com You had reviewed an earlier patch I had for this in https://bugs.webkit.org/show_bug.cgi?id=192495 (That patch was more of a work-around, less a fix) Digging in to find the actual issue I realized that my bug(192495) was a dupe of this one, so I moved the fix for the actual issue here.
Gabe Giosia
Comment 12 2019-04-23 15:48:06 PDT
Filed rdar://49352053
Ryosuke Niwa
Comment 13 2019-04-23 21:04:40 PDT
Antti/Zalan: could either one of you review this??
Gabe Giosia
Comment 14 2019-05-15 13:45:53 PDT
Friendly ping
Gabe Giosia
Comment 15 2019-05-17 08:04:04 PDT
This bug makes reliably measuring CJK content height nearly impossible and measuring the height of English content extremely slower(e.g. transforming English-language DOM so that it never hits a <br> that activates this condition is a slow operation).
Antti Koivisto
Comment 16 2019-05-20 13:04:41 PDT
r=me
WebKit Commit Bot
Comment 17 2019-05-20 13:32:41 PDT
Comment on attachment 363861 [details] Fix Range end offsets Clearing flags on attachment: 363861 Committed r245534: <https://trac.webkit.org/changeset/245534>
WebKit Commit Bot
Comment 18 2019-05-20 13:32:43 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 19 2019-05-20 13:33:18 PDT
<rdar://problem/50958290>
Note You need to log in before you can comment on or make changes to this bug.