Last Comment Bug 451212 - "Highlight all" interferes with the look of disabled normal selection
: "Highlight all" interferes with the look of disabled normal selection
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
-- minor (vote)
: mozilla1.9.2a1
Assigned To: Graeme McCutcheon [:graememcc]
: Mike de Boer [:mikedeboer]
Depends on: 476391
Blocks: 263683
  Show dependency treegraph
Reported: 2008-08-19 10:29 PDT by Martijn Wargers [:mwargers]
Modified: 2009-02-08 09:08 PST (History)
8 users (show)
hskupin: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Too much highlighted text (75.29 KB, image/png)
2008-08-23 09:02 PDT, Henrik Skupin (:whimboo) [away 02/18 - 02/27]
no flags Details
Trivial patch (700 bytes, patch)
2008-09-25 11:54 PDT, Graeme McCutcheon [:graememcc]
asaf: review+
Details | Diff | Splinter Review

Description User image Martijn Wargers [:mwargers] 2008-08-19 10:29:34 PDT
Not really a regression, but the patch for bug 263683 changed the behavior in a different broken state.

To reproduce:
- Do ctrl-A (or Edit->Select All from the menu)
- Ctrl-f search for a word or letter (which should be findable)
- Press the "Highlight all" button

Notice how some parts of the normal selection(depends on what text is highlighted by the find selection), become greyed out (which is correct, because the focus is in the chrome area) and other parts stay dark blue.
All of the normal selection should become greyed out.
Comment 1 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2008-08-19 11:08:10 PDT
When I hit Cmd+F and type the first letter inside the search bar the selection is reverted and no element is selected anymore. So I cannot reproduce it on OS X. Aleksej, something which happens on Linux?
Comment 2 User image Martijn Wargers [:mwargers] 2008-08-19 11:59:58 PDT
Sorry, step 1 and step 2 should be reversed. So first, do a ctrl-f search for a word, then do ctrl-a (Select All) (first focus the content area, to have ctrl-a have effect on the content).
Comment 3 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2008-08-23 09:01:58 PDT
That makes sense. I can reproduce it on OS X.
Comment 4 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2008-08-23 09:02:55 PDT
Created attachment 335179 [details]
Too much highlighted text
Comment 5 User image Graeme McCutcheon [:graememcc] 2008-08-23 16:31:16 PDT
Yeah, this is an interesting one.

The current behaviour stems from this line:

which is a relic from an old approach I was taking with bug 263683, and should definitely be removed. That line will be setting the selection display back from disabled to normal, but not explicitly repainting the selection. Hence, you see any nodes that contain a match flipping back to normal selection colours immediately, and other parts of the page will gradually change back haphazardly. Fun.

Simply removing the offending line from my findbar code will fix this, but gets us to an all new broken behaviour. Removing that line will mean that the user won't see the highlight until the content is next focused: all text will remain disabled, until you click in the content area, when you will then see the matches have indeed been highlighted). This lack of immediate feedback will no doubt confuse folk, making them think the highlighting hasn't worked. That can't be good.

So, fixing that second problem means moving from the findbar and putting our Gecko hats on.

As I understand it, the named constants in nsISelectionController.idl effectively define an order of precedence for painting selections - ie if there's a node with more than one type of selection on it, the higher numbered selection types are painted first, and then the lowered number types are painted over them. So, the solution would seem to be shuffling those numbers about to give SELECTION_FIND a higher precedence than SELECTION_NORMAL. 

However, this will hide the fact that when you search for a word, or use the next/previous buttons, the match is selected - the highlight will paint over the platform selection colours. 

Additionally I'm not sure what the implications are in terms of API breakage - arguably all consumers should be using the named constants as opposed to their current literal values, but as it's bad programming practice to depend on the literal values, you can bet there's some out there who do exactly that.

Rob: looking for some comments regarding what we would have to do regarding the selection controller interface. 

Mike, what do you think in terms of ui?
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-08-24 15:47:41 PDT
On Mac, I focus the content and then do "select all", then click in the find bar. The selection is grayed out because focus is now in the chrome.

But then I click "highlight all" and scroll the content around using the scrollbar, so everything's repainted with the blue selection as if the content is focused. Then I click in another application so the Firefox window is definitely not focused, but its selection is still blue, not grey.

Is that fixed by removing line 573?
Comment 7 User image Graeme McCutcheon [:graememcc] 2008-09-02 07:15:02 PDT
Yes and no! The problem you're finding in comment 6 is, but not the problem this bug is really about.

Remove line 573, follow your steps in comment 6, and yes the content in the selection will be grayed out, and will stay grayed out after you click highlight all, scroll around, open new apps etc.

Note however, that *everything* is grayed out - ie you cannot see any evidence of highlighting, despite the fact you have just clicked "highlight all". The highlighting is there, but you won't see it until you click in the content to focus it.

Hence, my comments in comment 5. Do we go with this, possibly confusing the user into thinking that highlighting hasn't worked? Or do we (and can we?) change around the order in which the selection types are painted, so that highlighting always works, regardless of the status of the normal selection?
Comment 8 User image Graeme McCutcheon [:graememcc] 2008-09-25 11:54:37 PDT
Created attachment 340379 [details] [diff] [review]
Trivial patch

The more I think about it, removing the offending line is the right thing to do anyway - the highlighting selection has no concept of ON or OFF, so the line is superfluous.

This will fix the bug as described: the changes in ux can perhaps be explored in another bug.
Comment 9 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-01-17 09:50:57 PST
Comment on attachment 340379 [details] [diff] [review]
Trivial patch

Looks ok to me, r=mano.
Comment 10 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-19 01:40:02 PST
Pushed befae055e824
Comment 11 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-02-01 15:27:35 PST
Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090201 Minefield/3.2a1pre ID:20090201020604

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090131 Minefield/3.2a1pre ID:20090131034630

Graeme, will the patch work for 1.9.1? Can we ask for approval? Further I think its worth to have an automated test here.

(In reply to comment #8)
> This will fix the bug as described: the changes in ux can perhaps be explored
> in another bug.

Filed that as bug 476391.
Comment 12 User image Graeme McCutcheon [:graememcc] 2009-02-05 17:22:59 PST
> Graeme, will the patch work for 1.9.1? Can we ask for approval? Further I think
> its worth to have an automated test here.

It would work for 1.9.1, and would be as low a risk a patch as you're going to get. That said, it's a bit of an edge-case that most people won't encounter - the STR are an uncommon workflow - so I have no strong feelings one way or the other about which release to get it in to. Anyone else have any thoughts?

With regards testing, not sure what to do here. I suppose one could synthesize some keypresses to simulate the STR, and compare the results from nsISelectionController.getDisplaySelection() before and after, verifying they are equal. However, to my mind, that doesn't really add any value, or really test this bug. What would really be needed would be a visual check, but it is pointless implementing that until the question of what should get painted gets resolved in 476391.
Comment 13 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-02-08 09:08:27 PST
Ok, that makes sense. So lets wait for bug 476391.

Note You need to log in before you can comment on or make changes to this bug.