1305033 - Emphasized highlight of symbols overlaps in matching results

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Simona Badau, Desktop QA]

Attachment: Screenshot with the issue

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20160922030437

[Affected versions]:
Nightly 52.0a2

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

[Steps to reproduce]:
1. Visit  https://reviewboard.mozilla.org/r/77950/diff/1/
2. Ctrl+F
3. Type: "rv)" or "mozilla {"
   (without any quotes)

[Expected result]:
- The searched matches are properly highlighted.

[Actual result]:
- The highlight is interrupted (mozilla+space+{) and overlaps the "a" from "mozilla".

[Regression range]:
- I'll investigate and post the results as soon as possible.

[Additional notes]:
- The issue is not reproducible if the preferences "findbar.modalHighlight" and "findbar.highlightAll" are set to false.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1305033 - make adjustments to the outline box style when one the ranges' rects overlap each other in the find toolbar dimmed highlighting mode.

Review commit: https://reviewboard.mozilla.org/r/83216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83216/

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797524 [details]
Bug 1305033 - make adjustments to the outline box style when one the ranges' rects overlap each other in the find toolbar dimmed highlighting mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83216/diff/1-2/

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797524 [details]
Bug 1305033 - make adjustments to the outline box style when one the ranges' rects overlap each other in the find toolbar dimmed highlighting mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83216/diff/2-3/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8797524 [details]
Bug 1305033 - make adjustments to the outline box style when one the ranges' rects overlap each other in the find toolbar dimmed highlighting mode.

https://reviewboard.mozilla.org/r/83216/#review82840

We should probably do the same for top and bottom margins, right?

::: toolkit/modules/FinderHighlighter.jsm:964
(Diff revision 3)
> +      // When left and/ or right sides will overlap with the current, previous
> +      // or next rect, make sure to make the necessary adjustments to the style.
> +      // These adjustments will override the styles as defined in `kModalStyles.outlineNode`.
> +      let intersectingSides = new Set();
> +      let previous = rects[i - 1];
> +      if (previous && previous.right + kOutlineBoxBorderSize >= rect.left - kOutlineBoxBorderSize)

if (previous &&
    rect.left - previous.right <= 2 * kOutlineBoxBorderSize) {
  intersectingSides.add("left");
}

::: toolkit/modules/FinderHighlighter.jsm:967
(Diff revision 3)
> +      if (next && next.left - kOutlineBoxBorderSize <= rect.right + kOutlineBoxBorderSize)
> +        intersectingSides.add("right");

if (next &&
    next.left - rect.right <= 2 * kOutlineBoxBorderSize) {
  intersectingSides.add("right");
}

::: toolkit/modules/FinderHighlighter.jsm:969
(Diff revision 3)
> +      let borderStyles = [...intersectingSides].map(side => [ "border-" + side, 0 ]);
> +      if (intersectingSides.size) {
> +        borderStyles.push([ "margin",  `-${kOutlineBoxBorderSize}px 0 0 ${
> +          intersectingSides.has("left") ? 0 : -kOutlineBoxBorderSize}px !important`]);
> +        borderStyles.push([ "border-radius",
> +          (intersectingSides.has("left") ? 0 : kOutlineBoxBorderRadius) + "px " +
> +          (intersectingSides.has("right") ? 0 : kOutlineBoxBorderRadius) + "px " +
> +          (intersectingSides.has("right") ? 0 : kOutlineBoxBorderRadius) + "px " +
> +          (intersectingSides.has("left") ? 0 : kOutlineBoxBorderRadius) + "px" ]);

It looks like this removes the margin on the sides that intersect. Instead of making these separate rects more obvious, we should be merging the rects.

In the example of:

rv)

I think it would be confusing to users to see [rv][)] as compared to [rv)]

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (overloaded with reviews / please needinfo? me) from comment #4)
> We should probably do the same for top and bottom margins, right?

I did that the first time around and that resulted very odd looking rects; it's certain that the sides may fully overlap each other, but the top and/ or bottom sides may only partially overlap. So they are rendered correctly the way it is now.

> It looks like this removes the margin on the sides that intersect. Instead
> of making these separate rects more obvious, we should be merging the rects.
> 
> In the example of:
> 
> rv)
> 
> I think it would be confusing to users to see [rv][)] as compared to [rv)]

That's why I'm removing the margin; I want the two rects to hug each other, without a gap in between. since the rects are merged, the positioning via the margin-left property is only necessary on the first rect, the margin-top will remain valid for all rects.
Or do you see this yields invalid results when you try this locally?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8797524 [details]
Bug 1305033 - make adjustments to the outline box style when one the ranges' rects overlap each other in the find toolbar dimmed highlighting mode.

https://reviewboard.mozilla.org/r/83216/#review83206

Okay, I thought it would just be better to grow the rects instead of trying to make them look like they are the same.

This is fine, but please make the other changes requested.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8797524 [details]
Bug 1305033 - make adjustments to the outline box style when one the ranges' rects overlap each other in the find toolbar dimmed highlighting mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83216/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d98cc2898db4
make adjustments to the outline box style when one the ranges' rects overlap each other in the find toolbar dimmed highlighting mode. r=jaws

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d98cc2898db4

Comment 10

[roxleitan]

Verified fixed using the latest Nightly 52.0a1 (Build ID:20161016030205) on Ubuntu 16.04, Windows 10 and Mac OS X 10.11.

I can still see an issue with the yellow highlight when zoom in/out the page. For this I filed bug 1310196.
Considering the above, setting the status verified fixed.