Closed Bug 1343695 Opened 3 years ago Closed 3 years ago

GetClientRectsAndTexts uses DOM text when it should use rendered text

Categories

(Core :: Layout, enhancement)

enhancement
Not set

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
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 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+
Attachment #8846790 - Flags: review?(mats)
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
Attachment #8846917 - Flags: review?(mats)
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)
(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 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+
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
You need to log in before you can comment on or make changes to this bug.