"Highlight all" interferes with the look of disabled normal selection

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Find Toolbar
--
minor
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: graememcc)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
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.
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?
(Reporter)

Comment 2

9 years ago
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).
That makes sense. I can reproduce it on OS X.
OS: Windows XP → All
Hardware: PC → All
Created attachment 335179 [details]
Too much highlighted text
(Assignee)

Comment 5

9 years ago
Yeah, this is an interesting one.

The current behaviour stems from this line:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml?mark=573#573

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?
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?
(Assignee)

Comment 7

9 years ago
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?
(Assignee)

Comment 8

9 years ago
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.
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
Attachment #340379 - Flags: review?(mano)
Comment on attachment 340379 [details] [diff] [review]
Trivial patch

Looks ok to me, r=mano.
Attachment #340379 - Flags: review?(mano) → review+
(Assignee)

Updated

9 years ago
Component: Selection → Find Toolbar
Keywords: checkin-needed
Product: Core → Toolkit
QA Contact: selection → fast.find
Pushed befae055e824
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Depends on: 476391
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 12

9 years ago
> 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.
Ok, that makes sense. So lets wait for bug 476391.
You need to log in before you can comment on or make changes to this bug.