Closed
Bug 1343695
Opened 7 years ago
Closed 7 years ago
GetClientRectsAndTexts uses DOM text when it should use rendered text
Categories
(Core :: Layout, enhancement)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla55
| Tracking | Status | |
|---|---|---|
| firefox55 | --- | fixed |
People
(Reporter: bradwerth, Assigned: bradwerth)
References
Details
Attachments
(3 files)
The GetClientRectsAndTexts function gets its return value from DOM text, which might not always match the rendered text. nsIFrame::GetRenderedText is the more correct implementation choice. See https://bugzilla.mozilla.org/show_bug.cgi?id=1314080#c39
| Comment hidden (mozreview-request) |
|
Assignee | |
Updated•7 years ago
|
Attachment #8842662 -
Flags: review?(bugs)
|
||
Comment 2•7 years ago
|
||
Comment on attachment 8842662 [details] Bug 1343695 Part 1: Retrieve text content with GetRenderedText. Since mats wanted this change, perhaps he could review this :)
Attachment #8842662 -
Flags: review?(bugs) → review?(mats)
|
||
Comment 3•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8842662 [details] Bug 1343695 Part 1: Retrieve text content with GetRenderedText. https://reviewboard.mozilla.org/r/116420/#review121322 LGTM, r=mats Please also add a "part 2" with some tests. Here's a few examples that would be good to test: <div style="width:0">a­b</div> <div style="width:100px">a­b</div> <div style="text-transform:uppercase">ab</div> <div>a b</div> <pre>a b</pre> (rs=mats on the test)
Attachment #8842662 -
Flags: review?(mats) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8846790 -
Flags: review?(mats)
|
|
||
Comment 6•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8846790 [details] Bug 1343695 Part 3: Expand tests of GetClientRectsAndTexts to test more cases where rendered text differs from DOM text. https://reviewboard.mozilla.org/r/119796/#review121678 Thanks! r=mats
Attachment #8846790 -
Flags: review?(mats) → review+
Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/687ffd715113 Part 1: Retrieve text content with GetRenderedText. r=mats https://hg.mozilla.org/integration/autoland/rev/e5db40a036fe Part 2: Expand tests of GetClientRectsAndTexts to test more cases where rendered text differs from DOM text. r=mats
I had to back these out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=83504308&repo=autoland https://hg.mozilla.org/integration/autoland/rev/41c24766c4ce
Flags: needinfo?(bwerth)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8846917 -
Flags: review?(mats)
|
|
Assignee | |
Comment 12•7 years ago
|
||
The assert at https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#9817 is the culprit. My read through the code leads me to believe it is not necessary. I tried removing it and the new test runs fine without it, still checking other tests on the try server https://treeherder.mozilla.org/#/jobs?repo=try&revision=42a307658375
Flags: needinfo?(bwerth)
|
|
||
Comment 13•7 years ago
|
||
I think you're right, at least as long as the given start offset isn't before
the start offset of 'this'. I'd prefer if we could relax the assertion just
enough so that our new call pass though. How about something like this:
MOZ_ASSERT(aStartOffset <= aEndOffset, "bogus offsets");
MOZ_ASSERT(!GetPrevContinuation() ||
(aOffsetType == TextOffsetType::OFFSETS_IN_CONTENT_TEXT &&
aStartOffset >= (uint32_t)GetContentOffset() &&
aEndOffset <= (uint32_t)GetContentLength()),
"Must be called on first-in-flow, or content offsets must be "
"given and be within this frame.");Flags: needinfo?(bwerth)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 17•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #13) > I think you're right, at least as long as the given start offset isn't before > the start offset of 'this'. I'd prefer if we could relax the assertion just > enough so that our new call pass though. How about something like this: Good idea. I've implemented your more narrow assert, with a small fix (replace GetContentLength with GetContentEnd).
Flags: needinfo?(bwerth)
|
|
||
Comment 18•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8846917 [details] Bug 1343695 Part 2: Narrow an overly restrictive assert in GetRenderedText. https://reviewboard.mozilla.org/r/119940/#review122214 > (replace GetContentLength with GetContentEnd). Ah, good catch. r=mats
Attachment #8846917 -
Flags: review?(mats) → review+
|
||
Comment 19•7 years ago
|
||
Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/da8e14542529 Part 1: Retrieve text content with GetRenderedText. r=mats https://hg.mozilla.org/integration/autoland/rev/9bf81510d7bc Part 2: Narrow an overly restrictive assert in GetRenderedText. r=mats https://hg.mozilla.org/integration/autoland/rev/31129448027b Part 3: Expand tests of GetClientRectsAndTexts to test more cases where rendered text differs from DOM text. r=mats
|
||
Comment 20•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/da8e14542529 https://hg.mozilla.org/mozilla-central/rev/9bf81510d7bc https://hg.mozilla.org/mozilla-central/rev/31129448027b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•