GetClientRectsAndTexts uses DOM text when it should use rendered text

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bradwerth, Assigned: bradwerth)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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

2 years ago
Attachment #8842662 - Flags: review?(bugs)
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

2 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&shy;b</div>
<div style="width:100px">a&shy;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

2 years ago
Attachment #8846790 - Flags: review?(mats)

Comment 6

2 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+

Comment 7

2 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8846917 - Flags: review?(mats)
(Assignee)

Comment 12

2 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)
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

2 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

2 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

2 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

2 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
Last Resolved: 2 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.