Open Bug 340814 Opened 15 years ago Updated 4 months ago

Highlighted text stays highlighted when closing the Find Toolbar

Categories

(Toolkit :: Find Toolbar, enhancement, P5)

1.8.0 Branch
enhancement

Tracking

()

People

(Reporter: NicolasWeb, Unassigned, Mentored)

References

Details

(Keywords: good-first-bug, parity-chrome, parity-safari, Whiteboard: [lang=js])

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

if you click "Highlight All" and then close the Find Toolbar, the highlighted text is still Highlighted.
This is confusing because you hide the button that control this function, and you always see the result. That's not very logic.

Reproducible: Always

Steps to Reproduce:
1.Open find toolbar and enter a word in the text box
2.Click the "Highlight All" button
3.Close the find toolbar

Actual Results:  
The highlighted text remain highlighted.
To unhighlight the text, you have to open again the find toolbar and "unselect" the "Highlight All" button.

Expected Results:  
Something more understanding (logic) for people. First ideas :
1. Stop to highlight the text when closing the find toolbar
2. Inform the user about this, and maybe ask him to choose. I think about the same alert than the anti phishing, or in the yellow top bar. maybe there is better ways to do that...
3. Ad an UI to disactivate the highlighting when the find toolbar is closed. a button, a context (pop up) menu item, an icon in the status bar ? some thing that appear when you mouse over an highlited word ?

I think that highlighted text stay highlighted when closing Find Toolbar is a function of Firefox, however it has to be improved.
To make matters worse:
* Highlight a term (e.g. "bug" on this page)
* Switch to another tab and switch back
* Search for another term (e.g. "high" on this page)

Result: both terms searched for are highlighted.

At least for Firefox 2, I suggest disabling highlighting when the search bar is hidden (and when another tab is selected in case the search bar is hidden too late in that case).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Oh well, that's bug 273304 - and it might get fixed for Firefox 2.

Still, disabling the highlighting when the find bar is closed would take care of some of the other issues it causes and would make it clear that highlighting is a function of the find bar (which doesn't close automatically, so the user can still choose when to see the highlighted text and when not).
Well, for people using low-resolution screens (old PCs, PDAs and tiny laptops), the being able to highlight stuff then hide the find bar might be quite desirable, to gain the maximum screen real estate while still being able to highlight stuff.

So I'm not sure if this is actually a good idea after all.
Product: Firefox → Toolkit
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → 1.8.0 Branch
Duplicate of this bug: 483210
Even if bug  273304  is RESOLVED FIXED this still happen in Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; fr; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6
Blocks: cuts-cruft
Version: 1.8.0 Branch → unspecified
Version: unspecified → 1.8.0 Branch
Why if this happen in 1.9.2.6 you mark it on 1.8.0 branch ? ;-)
Under Firefox and Toolkit the version field is used to indicate the first version where the bug has been reported against.
No longer blocks: cuts-cruft
the problem is still continue with FF 9.0.1 find toolkit.
Still current in Aurora 25.
using Firefox 26 on mac. the bug still exists.
Duplicate of this bug: 1037888
I suspect fixing this will break somebody's workflow but it does make us feel kinda broken.
Flags: firefox-backlog?
Whiteboard: [parity-safari][parity-chrome]
Summary: Highlighted text stay highlighted when closing Find Toolbar → Highlighted text stays highlighted when closing the Find Toolbar
Flags: firefox-backlog? → firefox-backlog+
Duplicate of this bug: 1056806
Mentor: felipc
Whiteboard: [parity-safari][parity-chrome] → [parity-safari][parity-chrome][good first bug][lang=js]
Mentor: mdeboer
Felipe, I'm taking the bug as we discussed.

Can you point me to those files you mentioned?
Assignee: nobody → mv.nsaad
Attached patch v1.patch (obsolete) — Splinter Review
This patch clears highlighting on nodes when the Find Bar is closed.
Attachment #8536869 - Flags: review?(felipc)
Comment on attachment 8536869 [details] [diff] [review]
v1.patch

Review of attachment 8536869 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Marcus, thanks for the patch!

This looks right to me, but I'll delegate the review to Mike de Boer who has done a lot more work on the findbar than me.
Attachment #8536869 - Flags: review?(mdeboer)
Attachment #8536869 - Flags: review?(felipc)
Attachment #8536869 - Flags: review+
I thought this was by design. Is is really a good thing to fix this?
(In reply to Masatoshi Kimura [:emk] from comment #17)
> I thought this was by design. Is is really a good thing to fix this?

No, this is broken, see comment 1.
(In reply to José Jeria from comment #18)
> (In reply to Masatoshi Kimura [:emk] from comment #17)
> > I thought this was by design. Is is really a good thing to fix this?
> 
> No, this is broken, see comment 1.

I can't reproduce the bug explained in comment #1.
#18, steps to reproduce:

1 - Open find bar, type a search that will find more than 1 result.
2 - Click on highlight all
3 - Close find bar.

Result:
Highlighted words are still highlighted after find bar is closed

Expected result:
Highlighted words are no longer highlighted after find bar is closed
It is a different "issue" from comment #1 which is saying multiple words will be highlighted.
And comment #18 is the exactly what I (and some other comments) said "by design".
(In reply to Masatoshi Kimura [:emk] from comment #21)
> It is a different "issue" from comment #1 which is saying multiple words
> will be highlighted.

Regarding comment #1, this apparently got fixed in bug 273304 according to comment #2. Maybe José and Marcus were referring to the Description (which apparently is linked as 'comment #0' ;)

> I thought this was by design. Is is really a good thing to fix this?

I agree with Matthew there from comment #12

> I suspect fixing this will break somebody's workflow but it does make us
> feel kinda broken.
Comment on attachment 8536869 [details] [diff] [review]
v1.patch

Review of attachment 8536869 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Like Felipe said, thanks for the patch Markus!

To answer ppl worried whether this change is for the better: with Highlight All on and matching multiple occurrences in a document, leaving the (fuchsia-colored) ranges active after closing the findbar leaves them without context. The user can _not_ clear these fuchsia blocks other than re-opening the findbar, clearing the input field inside or deselecting the Highlight All toggle button.

These two reasons make Marcus' patch a better way forward than the suboptimal status quo.
Attachment #8536869 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/integration/fx-team/rev/95f5bf018706

We had to backout the patch because it caused a test failure. Probably a test that was relying on the old behavior and now needs to be updated. Marcus, can you take a look?

See the log here: https://tbpl.mozilla.org/php/getParsedLog.php?id=54885829&tree=Fx-Team#error0

To run the test, use:
  mach mochitest-chrome test_bug451286.xul

This is the full path of the test:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/test_bug451286.xul
But it's one of those tests that opens another window and all the test code is in this other file:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/bug451286_window.xul

Ping me on IRC if you need any help!
Feel free to email or ping me on IRC as well!
Status: NEW → ASSIGNED
Thanks for the support guys, I feel right at home with mochitests :)

Due to the changes introduced on this patch, a call on line #117 on http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/bug451286_window.xul#117, will cause the highlighted words to be unmarked when the findbar is closed, therefore both snapshots will provide different outputs.

What would be preferred, to change the snapshot we compare to, or to assert:
 
isnot(res[0], true, "Matches found in iframe incorrectly highlighted")
(In reply to Marcus Saad (:msaad) from comment #27)
> Thanks for the support guys, I feel right at home with mochitests :)
> 
> Due to the changes introduced on this patch, a call on line #117 on
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/
> bug451286_window.xul#117, will cause the highlighted words to be unmarked
> when the findbar is closed, therefore both snapshots will provide different
> outputs.
> 
> What would be preferred, to change the snapshot we compare to, or to assert:
>  
> isnot(res[0], true, "Matches found in iframe incorrectly highlighted")

I think it makes more sense to take the snapshot at a more appropriate time during the test, or else the test file will become less and less understandable... Do you agree?
Attached patch v2.patch (obsolete) — Splinter Review
>       // Turn on highlighting
>       yield toggleHighlightAndWait(true);
> -     yield closeFindbarAndWait();
>     });

Removed the call to closeFindbarAndWait() because it would clean the highlighted words

>-      // Now, remove the highlighting, and take a snapshot to compare
>-      // to our original state
>-      gFindBar.open();
>-      yield toggleHighlightAndWait(false);
>+      // Due to bug 340814, closing FindBar will remove higlighted words
>       yield closeFindbarAndWait();

Removed the call to gFindBar.open() because it was already opened. Removed the call to toggleHighlightAndWait() because it will be unhighlighted when closing. Added a comment and relocated the call to closeFindbarAndWait().


>+      // Opening FindBar so both snapshots have the same size
>+      gFindBar.open();
>       // Take snapshot of manual highlighting
>       var manualSnapshot = snapshotWindow(gBrowser.contentWindow);

At last, we open the FindBar so that both snapshots have the same size and comparing them makes sense.
Attachment #8536869 - Attachment is obsolete: true
Attachment #8539465 - Flags: review?(mdeboer)
I tested the try build.
When I pressed "Highlight All", closed the find bar and opened it again, the word was not highlighted while "Highlight All" is pressed. I had to toggle "Highlight All" again to get the highlight. Is this expected? Hopefully it is not...
Although Chrome had the same behavior, IE 11 had no problem. (Chrome is worse than the try build in this regard. Chrome's find bar does not have the "Highlight All" button, so I have no way to workaround this.)
(In reply to Masatoshi Kimura [:emk] from comment #31)
> I tested the try build.
> When I pressed "Highlight All", closed the find bar and opened it again, the
> word was not highlighted while "Highlight All" is pressed. I had to toggle
> "Highlight All" again to get the highlight. Is this expected? Hopefully it
> is not...

Marcus, can you see if fixing this behavior would be easy to implement for you? Thanks!
Flags: needinfo?(mv.nsaad)
Other than that, the patch looks good to me.
Sorry for the delay, was on vacation. Of course, I will attempt to fix that as well. 

Mike and Felipe, what are your opinions on cleaning the search text input too when the find bar is closed? *I'm not proposing it*, I'm just playing with the idea since we are moving from a stateful find bar (used to keep text highlighted when closed) to a stateless one (text isn't kept highlighted any more after it is closed). Does it make sense to clean it's state when closed? 

Will hopefully come up with a patch in the next few days.
Flags: needinfo?(mv.nsaad)
(In reply to Marcus Saad (:msaad) from comment #34)
> Mike and Felipe, what are your opinions on cleaning the search text input
> too when the find bar is closed? *I'm not proposing it*, I'm just playing
> with the idea since we are moving from a stateful find bar (used to keep
> text highlighted when closed) to a stateless one (text isn't kept
> highlighted any more after it is closed). Does it make sense to clean it's
> state when closed? 

I understand that this'd make sense technically, but this would break the workflow of many users; some/ many use it as a navigation tool, just like a coder does in their favorite IDE, and that's one of the reasons why the find bar is such an important component for a navigator like Firefox.
Comment on attachment 8539465 [details] [diff] [review]
v2.patch

I'd be happy to take a look at your v3 patch, Marcus!
Attachment #8539465 - Flags: review?(mdeboer)
Attached patch v3.patchSplinter Review
Hope this patch nails it. 

Can you review it for me Mike?

Thanks in advance
Attachment #8539465 - Attachment is obsolete: true
Attachment #8545520 - Flags: review?(mdeboer)
Comment on attachment 8545520 [details] [diff] [review]
v3.patch

Review of attachment 8545520 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/findbar.xml
@@ +631,5 @@
>            this._findField.blur();
>  
>            this._cancelTimers();
> +          this.toggleHighlight(false);
> +          this.getElement("highlight").checked = false;

This *looks* like a hack to me... what's your rationale for doing this?
Attachment #8545520 - Flags: review?(mdeboer)
Mike,

Looking on the same file (toolkit/content/widgets/findbar.xml), I found examples of other properties being changed through DOM manipulation, like on line 527, 528 where the hidden property is being changed, so I thought it would be ok to manipulate it directly. I'm sorry if if what I did isn't the most appropriate way.

Would you recommend me something else?
(In reply to Marcus Saad (:msaad) from comment #39)
> Mike,
> 
> Looking on the same file (toolkit/content/widgets/findbar.xml), I found
> examples of other properties being changed through DOM manipulation, like on
> line 527, 528 where the hidden property is being changed, so I thought it
> would be ok to manipulate it directly. I'm sorry if if what I did isn't the
> most appropriate way.
> 
> Would you recommend me something else?

Marcus, I'm sorry for not being more specific, I shouldn't have left that note in a hurry!

So what I think would be more appropriate is to add the following to the findbar 'open' method:

if (this.browser._lastSearchHighlight)
  this.toggleHighlight(this.browser._lastSearchHighlight);

... right at the end of the `if (this.hidden) {` block in the 'open' method. What do you think?
No need to apologize Mike.

I have tried the code you suggested but could not get the desired effect of cleaning what currently is highlighted and unchecking the button. 

Chances are that I have done something wrong. Have you tested on your end? If so, I'll post a patch with it.
Priority: -- → P5
Works for me in 58.0.2. However, see a related issue in Bug 1445901.
See Also: → 1445901, 803456
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Whiteboard: [parity-safari][parity-chrome][good first bug][lang=js] → [good first bug][lang=js]
Keywords: good-first-bug
Whiteboard: [good first bug][lang=js] → [lang=js]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: mv.nsaad → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.