1300824 - Backspace on a match shows the white selection briefly

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

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

STR:
Open data:text/html,this is a test
Search for "test"
Hit backspace

The white selection underneath the yellow highlight shows for a a couple frames before they it is removed.

We should probably not color the selection under the highlight or at least color it the same as the dim.

Comment 1

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

Attachment: Screenshot of bug

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1300824 - skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field.

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

Comment 3

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

Comment on attachment 8789753 [details]
Bug 1300824 - skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field.

https://reviewboard.mozilla.org/r/77852/#review76332

r- for changing how inflate works because I don't think you intended it like this. The rest of the changes look fine.

::: toolkit/modules/FinderHighlighter.jsm:803
(Diff revision 1)
> +    return rects;
> +  },
>  
> +  /**
> +   * Read and store the rectangles that encompass the entire region of a range
> +   * for use by the drawing function of the highlighter an store them in the

s/an/and/

::: toolkit/modules/Geometry.jsm:326
(Diff revision 1)
>     * Grows or shrinks the rectangle while keeping the center point.
> -   * Accepts single multipler, or separate for both axes.
> +   * Accepts single multiplier, or separate for both axes.
>     */
> -  inflate: function inflate(xscl, yscl) {
> +  inflateToMultiple(xscl, yscl) {
>      let xAdj = (this.width * xscl - this.width) / 2;
>      let s = (arguments.length > 1) ? yscl : xscl;

You should switch yscl to have a default value of xscl like you did in inflate() and then you can get rid of this line.

::: toolkit/modules/Geometry.jsm:332
(Diff revision 1)
> +   * Grows or shrinks the rectangle while keeping the center point.
> +   * Accepts single real number adjustment, or separate for both axes.
> +   */
> +  inflate(xAdj, yAdj = xAdj) {

This changes how inflate works. Before if xscl was 5, it would make xAdj equal to 2.5. But now if inflate is called with the first argument of 5, it will actually apply a size change 5 to each side, not 2.5.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> You should switch yscl to have a default value of xscl like you did in
> inflate() and then you can get rid of this line.

Will do!

> This changes how inflate works. Before if xscl was 5, it would make xAdj
> equal to 2.5. But now if inflate is called with the first argument of 5, it
> will actually apply a size change 5 to each side, not 2.5.

Not entirely correct; with the previous method accepted xscl as a multiplier - it doesn't just divide xscl by 2 - and was introduced by Fennec in https://hg.mozilla.org/mozilla-central/rev/329bd5d87779. This, however, is not used anywhere anymore (not even by Fennec) and doesn't correspond with the semantics present in gfx/2d/BaseRect.h, which is the only real code using geometry I've found in m-c.
Therefore I don't think there's a backward compat risk in changing this API and in fact will make it more understandable for future contributors.
I borrowed the name 'inflateToMultiple' from 'gfx/2d/Rect.h'.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8789753 [details]
Bug 1300824 - skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field.

Apologies for not telling this story sooner and expecting from you to dive in without the context I already had spending 2hrs on this :-/

Comment 6

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

Comment on attachment 8789753 [details]
Bug 1300824 - skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field.

https://reviewboard.mozilla.org/r/77852/#review76374

::: toolkit/modules/Geometry.jsm:333
(Diff revision 1)
> +   * Accepts single real number adjustment, or separate for both axes.
> +   */
> +  inflate(xAdj, yAdj = xAdj) {
>      this.left -= xAdj;
>      this.right += xAdj;

Okay, but this is still a bit confusing, no?

let rect = Rect.fromRect{left: 0, right: 100, top: 0, bottom: 100});
rect.inflate(10);

Should that give me a Rect that is now 120px wide or 110px wide?

Also, ", or separate for both axis" here is misleading. It's really "separate for different values on each axis" because a single number does apply the value to both axis.

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Comment on attachment 8789753 [details]
> Bug 1300824 - skip cutting out the current found range from the mask so it
> won't show when you backspace in the findbar input field.
> 
> https://reviewboard.mozilla.org/r/77852/#review76374
> 
> ::: toolkit/modules/Geometry.jsm:333
> (Diff revision 1)
> > +   * Accepts single real number adjustment, or separate for both axes.
> > +   */
> > +  inflate(xAdj, yAdj = xAdj) {
> >      this.left -= xAdj;
> >      this.right += xAdj;
> 
> Okay, but this is still a bit confusing, no?
> 
> let rect = Rect.fromRect{left: 0, right: 100, top: 0, bottom: 100});
> rect.inflate(10);
> 
> Should that give me a Rect that is now 120px wide or 110px wide?

Ah! Yeah, you're right. I need to /= 2.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8789753 [details]
Bug 1300824 - skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field.

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

Comment 9

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

Comment on attachment 8789753 [details]
Bug 1300824 - skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field.

https://reviewboard.mozilla.org/r/77852/#review76648

Thanks, this is a much simpler patch now.

::: toolkit/modules/FinderHighlighter.jsm:445
(Diff revision 2)
>      if (this._highlightAll && data.searchString)
>        this.highlight(true, data.searchString, data.linksOnly);
>    },
>  
>    /**
>     * Invalidates the list by clearing the map of highglighted ranges that we

s/highglighted/highlighted/

::: toolkit/modules/FinderHighlighter.jsm:476
(Diff revision 2)
>     */
>    onLocationChange() {
>      let window = this.finder._getWindow();
>      let dict = this.getForWindow(window);
>      this.clear(window);
> +    dict.currentFoundRange = null;

I'm not sure why this line moved out of clear(), since clear() is called right before this. Should we not clear the currentFoundRange each time clear() is called?

::: toolkit/modules/FinderHighlighter.jsm:748
(Diff revision 2)
>      return false;
>    },
>  
>    /**
>     * Read and store the rectangles that encompass the entire region of a range
> -   * for use by the drawing function of the highlighter.
> +   * for use by the drawing function of the highlighter an store them in the

s/an/and/

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> I'm not sure why this line moved out of clear(), since clear() is called
> right before this. Should we not clear the currentFoundRange each time
> clear() is called?

No, because that means that _updateRangeOutline() would not have a range anymore at the next repaint, resulting in an empty yellow box.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8789753 [details]
Bug 1300824 - skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field.

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

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8789753 [details]
Bug 1300824 - skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field.

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

Comment 13

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cb0ca133b1e

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8789753 [details]
Bug 1300824 - skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7e88c3a5541
skip cutting out the current found range from the mask so it won't show when you backspace in the findbar input field. r=jaws

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

I've reproduced the initial issue on Nightly 51 Build ID: 20160914030200 on Ubuntu 16.04, Mac OS X 10.11 and Windows 10.

Verified fixed on latest Nightly 51, build ID:20160915030417 (used the scenario from the Description and the scenario from the duplicate bug 1302207) on Ubuntu 16.04, Mac OS X 10.11 and Windows 10.