Closed Bug 1334088 Opened 7 years ago Closed 7 years ago

[New Find Toolbar] The highlighted text changes the original position

Categories

(Toolkit :: Find Toolbar, defect)

54 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox54 --- verified

People

(Reporter: roxana.leitan, Assigned: bradwerth)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files)

Attached image bad.png
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID:20170124030205

[Affected versions]:
Nightly 54.0a1

[Affected platforms]:
All: Windows 10 x 64, Ubuntu 16.04, Mac OS X 10.11

[Steps to reproduce]:
1.Open Nightly
2.Go to http://www.nytimes.com/2016/06/28/world/europe/brexit-bregret-european-union-the-interpreter.html?_r=0
3.Press CTRL+F to open Find toolbar
4.Search for a long string that appears on this page and contains links

[Expected Result]:
The String should be found and highlighted with yellow. The highlight should respect the structure of the string and the text should remain in the original position.

[Actual Result]:
The string is highlighted but the text changed the original position(the 2nd and 3rd rows are the same)
Assignee: nobody → bwerth
Confirmed that this is a bug in the CollectClientRectsAndText code I wrote for bug 1302470. When ready, fix will appear there.
Blocks: 1302470
(In reply to Brad Werth [:bradwerth] from comment #1)
> Confirmed that this is a bug in the CollectClientRectsAndText code I wrote
> for bug 1302470. When ready, fix will appear there.

Whoops, the patch for bug 1314080 contains the offending code.
Attachment #8836255 - Flags: review?(dholbert)
In testing this, I discovered that searches that begin with a space also display incorrectly, in that the highlighted text omits the space. Mike will likely be better able to make the JS changes needed for this part of the bug.
(In reply to Brad Werth [:bradwerth] from comment #4)
> In testing this, I discovered that searches that begin with a space also
> display incorrectly, in that the highlighted text omits the space. Mike will
> likely be better able to make the JS changes needed for this part of the bug.

Might be related to bug 1311742 (which is about spaces being shifted around in find-results).
Comment on attachment 8836255 [details]
Bug 1334088 Part 1: Correct BoxToRectAndText to respect content offset and length when returning text.

https://reviewboard.mozilla.org/r/111722/#review113032

::: layout/base/nsLayoutUtils.cpp:3981
(Diff revision 1)
>        nsIContent* content = aFrame->GetContent();
>        nsAutoString textContent;
>        mozilla::ErrorResult err; // ignored
>        content->GetTextContent(textContent, err);

With your new code, it's likely we'll return before we ever even use |textContent|.

So, probably best to insert your new code *before* we bother looking up |content| & |textContent| so that these variables are declared closer to their usage.

::: layout/base/nsLayoutUtils.cpp:3986
(Diff revision 1)
> +      // Find the appropriate substring to match the rect.
> +      // To do this, we need to get the nsTextFrame that contains the text that
> +      // was matched (and has the offsets relative to textContent).
> +      for (nsIFrame* child : aFrame->PrincipalChildList()) {
> +        if (child->GetType() == nsGkAtoms::textFrame) {
> +          nsTextFrame* textFrame = static_cast<nsTextFrame*>(child);
> +          const nsAString& textSubstring =
> +            Substring(textContent,
> +                      textFrame->GetContentOffset(),
> +                      textFrame->GetContentLength());
> +          mTextList->Add(textSubstring);
> +          return;

It looks like this just finds aFrame's first nsTextFrame child, adds that child's text-contents, and then returns.

But what aFrame has multiple nsTextFrame children? Why do we only care about the first one?  Why are we disregarding all the rest, when we got this AddBox() call for aFrame itself?

(I'm not really familiar with this BoxToRectAndText API, so it's entirely possible that this is reasonable but I just don't understand it yet. :))
Attachment #8836255 - Flags: review?(dholbert) → review-
Comment on attachment 8836255 [details]
Bug 1334088 Part 1: Correct BoxToRectAndText to respect content offset and length when returning text.

https://reviewboard.mozilla.org/r/111722/#review113032

> It looks like this just finds aFrame's first nsTextFrame child, adds that child's text-contents, and then returns.
> 
> But what aFrame has multiple nsTextFrame children? Why do we only care about the first one?  Why are we disregarding all the rest, when we got this AddBox() call for aFrame itself?
> 
> (I'm not really familiar with this BoxToRectAndText API, so it's entirely possible that this is reasonable but I just don't understand it yet. :))

er, I meant "But what *if* aFrame has" (not "But what aFrame has")
Comment on attachment 8836255 [details]
Bug 1334088 Part 1: Correct BoxToRectAndText to respect content offset and length when returning text.

https://reviewboard.mozilla.org/r/111722/#review113040

::: layout/base/nsLayoutUtils.cpp:3981
(Diff revision 1)
>        nsIContent* content = aFrame->GetContent();
>        nsAutoString textContent;
>        mozilla::ErrorResult err; // ignored
>        content->GetTextContent(textContent, err);

The code uses textContent in all paths (source of the substring in the early-exit path).
Comment on attachment 8836255 [details]
Bug 1334088 Part 1: Correct BoxToRectAndText to respect content offset and length when returning text.

https://reviewboard.mozilla.org/r/111722/#review113054

::: layout/base/nsLayoutUtils.cpp:3986
(Diff revision 1)
> +      // Find the appropriate substring to match the rect.
> +      // To do this, we need to get the nsTextFrame that contains the text that
> +      // was matched (and has the offsets relative to textContent).
> +      for (nsIFrame* child : aFrame->PrincipalChildList()) {
> +        if (child->GetType() == nsGkAtoms::textFrame) {
> +          nsTextFrame* textFrame = static_cast<nsTextFrame*>(child);
> +          const nsAString& textSubstring =
> +            Substring(textContent,
> +                      textFrame->GetContentOffset(),
> +                      textFrame->GetContentLength());
> +          mTextList->Add(textSubstring);
> +          return;

You're right, and it's possible this is only working because this generic-sounding method is actually only used in a very specific case (the pending typeahead-find code). At least part of the solution will be for me to build a bunch of tests that try different ranges across differently-structured content. That will flush out any cases where this logic fails.

To address the intent, the code wants the the offset and length into the content that corresponds to the rect of aFrame. In debugging, I saw that data was available in the first child nsTextFrame in my test case, but I'll look for a way to get those values more generally.
(In reply to Brad Werth [:bradwerth] from comment #4)
> In testing this, I discovered that searches that begin with a space also
> display incorrectly, in that the highlighted text omits the space. Mike will
> likely be better able to make the JS changes needed for this part of the bug.

Adding needinfo for Mike.
Flags: needinfo?(mdeboer)
OK, that'd indeed be a bug for me to tackle! I think it's best to separate that out from this bug so you can focus on the work here.
Flags: needinfo?(mdeboer)
What I meant to ask was: Brad, can you file a bug for me that blocks bug 1291278 and contains a screenshot and/ or STR of the issue? Thanks!
Attachment #8838290 - Flags: review?(dholbert)
(In reply to Mike de Boer [:mikedeboer] from comment #12)
> What I meant to ask was: Brad, can you file a bug for me that blocks bug
> 1291278 and contains a screenshot and/ or STR of the issue? Thanks!

Filed. Bug 1340731.
See Also: → 1340731
Sorry for not having gotten to this yet!

I've been putting it off because (a) I'm unfamiliar with this code and (b) I had a 24-part review request on another bug that I've been front-loading my review cycles towards, to avoid 24-part-bitrot. :D

jfkthame, perhaps you'd be up for stealing review here? You're probably more likely to catch issues with text-related APIs than I am. Alternately, if this is still open when I'm back from vacation on Monday, I'll definitely take a look then.
Flags: needinfo?(jfkthame)
Comment on attachment 8836255 [details]
Bug 1334088 Part 1: Correct BoxToRectAndText to respect content offset and length when returning text.

https://reviewboard.mozilla.org/r/111722/#review116416

Seems reasonable to me (though in the process of looking at this, I noticed another problem with the Find highlight - filed as bug 1342080).
Attachment #8836255 - Flags: review+
Comment on attachment 8838290 [details]
Bug 1334088 Part 2: Add tests of Range.getClientRectsAndTexts.

https://reviewboard.mozilla.org/r/113244/#review116408

::: layout/base/tests/chrome/test_getClientRectsAndTexts.html:10
(Diff revision 3)
> +<div id="div1" style="width:200px"
> +>Here is some text that <a href="#">will wrap</a> in <a href="#">this small</a>-ish container.</div
> +><div id="div2"
> +>Into another <a href="#">container</a></div
> +><div id="div3"
> +>A very <span>deep <span>deep <span>deep</span></span></span> bit of text.</div>

AFAICT, it's not necessary to "hide" the line breaks inside the closing `</div>` tags like this, and I think it makes the test more confusing to look at (and less typical of real content).

Writing this in a more natural form as

    <div id="div1" style="width:200px">Here is some text that <a href="#">will wrap</a> in <a href="#">this small</a>-ish container.</div>
    <div id="div2">Into another <a href="#">container</a></div>
    <div id="div3">A very <span>deep <span>deep <span>deep</span></span></span> bit of text.</div>

should work equally well, I think.

::: layout/base/tests/chrome/test_getClientRectsAndTexts.html:78
(Diff revision 3)
> +for (let d = 0; d < data.length; d++) {
> +  testRangeTexts.apply(null, data[d]);
> +}

Maybe more idiomatic as

    data.forEach(function (d) { testRangeTexts.apply(null, d); });
Attachment #8838290 - Flags: review+
(In reply to Jonathan Kew (:jfkthame) from comment #22)
> ::: layout/base/tests/chrome/test_getClientRectsAndTexts.html:78
> (Diff revision 3)
> > +for (let d = 0; d < data.length; d++) {
> > +  testRangeTexts.apply(null, data[d]);
> > +}
> 
> Maybe more idiomatic as
> 
>     data.forEach(function (d) { testRangeTexts.apply(null, d); });

[Drive-by] Or even:
```js
for (let d of data)
  testRangeTexts(...d);
```
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22b0c9160df8
Part 1: Correct BoxToRectAndText to respect content offset and length when returning text. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/88a7b28e2b71
Part 2: Add tests of Range.getClientRectsAndTexts. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/22b0c9160df8
https://hg.mozilla.org/mozilla-central/rev/88a7b28e2b71
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Attachment #8836255 - Flags: review?(dholbert)
Attachment #8838290 - Flags: review?(dholbert)
Verified on the latest Nightly 54.0a1 (2017-02-27) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11. 
The initial issue is gone, the highlight respects the structure of the string, but still see one issue filed in bug 1343198.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.