Open Bug 547218 Opened 11 years ago Updated 5 years ago

Findbar highlighting should not be stored in bfcache

Categories

(Toolkit :: Find Toolbar, defect, P5)

defect

Tracking

()

ASSIGNED

People

(Reporter: graememcc, Assigned: graememcc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Whilst it makes sense for normal selections to be stored in bfcache, preserving the find selections created with findbar highlighting makes less sense. The findbar should take steps to remove it's highlighting in this case.
Attached patch Patch v1 (obsolete) — Splinter Review
In terms of UX, the only use-case I think we would break by doing this, is when someone highlights a search term, inadvertently clicks forward on a link, and navigates back and wants their highlights to still be there - and I think it would be a stretch to call that a common use-case.

In addition, doing this would make fixing 279022 - finally! - trivially simple, no more messing around with WebProgressListeners that regressed Txul.
Attachment #427869 - Flags: review?(mano)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Spotted an obvious bit of refactoring in the test.
Attachment #427869 - Attachment is obsolete: true
Attachment #427973 - Flags: review?(mano)
Attachment #427869 - Flags: review?(mano)
A couple of this before I'm reviewing this:

1. Did you test this with frames and iframes? Shouldn't we "filter out" pagehide events coming from these sub-documents?
2. Please use "let" (and not "var") in any new js code.
Comment on attachment 427973 [details] [diff] [review]
Patch v1.1

>+          // Remove highlighting from any highlighted pages in the browser
>+          var browsers = [];

var browsers;

>+          if (this.browser.localName == "browser")
>+            browsers = [this.browser];
>+          else if (this.browser.localName == "tabbrowser")
>+            browsers = this.browser.browsers;
(In reply to comment #5)
> (From update of attachment 427973 [details] [diff] [review])
> >+          // Remove highlighting from any highlighted pages in the browser
> >+          var browsers = [];
> 
> var browsers;
> 
> >+          if (this.browser.localName == "browser")
> >+            browsers = [this.browser];
> >+          else if (this.browser.localName == "tabbrowser")
> >+            browsers = this.browser.browsers;

I guess you're protecting against 'browser' being neither a browser nor a tabbrowser, but does that actually make sense? I'd suggest this instead:

if (this.browser.localName == "tabbrowser")
  var browsers = this.browser.browsers;
else
  browsers = [this.browser];

Or even this:

if ("browsers" in this.browser)
  var browsers = this.browser.browsers;
else
  browsers = [this.browser];
Attached patch Patch v1.2Splinter Review
> ping
Apologies for being out for so long. Lack of spare time there for a while.

. Did you test this with frames and iframes? Shouldn't we "filter out"
pagehide events coming from these sub-documents?
2. Please use "let" (and not "var") in any new js code.

2 - Done.
1 - In retooling the tests (amongst other things) I added some coverage here. 

I've been arguing with myself over whether to just unhighlight on the top-level window's pagehide. At the moment, we don't cache subframe navigations by default. However, you can switch it on at the flip of a pref. I believe you wouldn't want to as it is pretty buggy, but someday that might get fixed.

Say it does get fixed. You have a page with an iframe, where both the top-level doc and the frame contain a match for the search term, which you highlight. The doc changes the contents of the frame somehow. You switch off highlighting, and click back in the frame's context menu. If we're not listening to pagehides on the individual frames, then the old frame's coming back with highlighting intact, and you then have to rehighlight the doc and switch it off again to get rid of it.

Yeah, we instead get the opposite problem - if the rest of the doc's still highlighted, the frame content's are going to come back unhighlighted - but we're at least being consistent in a no-highlighting-ends-up-in-the-cache kind of way.

And of course, all of the last 3 paragraphs are moot until subframe-navigation caching gets switched on, but I'd rather just write this code once and forget about it!

> I guess you're protecting against 'browser' being neither a browser nor a
> tabbrowser, but does that actually make sense? I'd suggest this instead:
...
> Or even this:
...

I blew all that away so as not to couple the findbar to tabbrowser properties. I'm choosing now to hold on to a reference to the window. I feel comfortable with this, given that if we highlight it, we will attach a pagehide, so we'll get notified in a timely manner to stop holding our reference.

One thing I am concerned about in the tests is that counting and comparing the number of pagehide listeners across navigations may be fragile.
Attachment #427973 - Attachment is obsolete: true
Attachment #448216 - Flags: review?(mano)
Attachment #427973 - Flags: review?(mano)
Graeme, I'm really sorry I haven't got to this on time. Should I still review this?

Again, sorry!
Comment on attachment 448216 [details] [diff] [review]
Patch v1.2

For future reference: in a few discussions over the years about highlighting bugs, the conclusion is always the same: highlighting should be reimplemented using selections, so the DOM is not affected (the same way spelling is implemented).
Attachment #448216 - Flags: review?(mano)
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.