451252 - "Highlight All" find selection not visible in elements that have -moz-user-select: none set

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

Again, formally a regression, but practically this is only happening because of the switch to the use of selection.
This already didn't work for the normal find.

To reproduce:
- Ctrl-f, type in the word 'mozilla'
- Click on "Highlight All"

Only the first 'mozilla' word is highlighted.

It seems to me -moz-user-select should only have an effect on the normal selection, not on any other selection type.

Comment 1

[Graeme McCutcheon [:graememcc]]

So, that'll be the IsSelectable computations in the SetSelected method of the relevant frames stopping this working.

> It seems to me -moz-user-select should only have an effect on the normal
> selection, not on any other selection type.

To achieve this, I'm imagining this would require passing in the selection type as a parameter to the frame SetSelected methods. Yuck.

roc, any thoughts?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds right. Is it really yuck?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or, would it make sense to just ignore IsSelectable in the SetSelected methods? It's OK to invalidate and/or reflow unnecessarily. The only important thing is that we don't paint the selection if it's not supposed to be selectable.

Comment 4

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v1

> Is it really yuck?

OK, maybe I was being a little dramatic :)

Just seems to be a lot of extra parameters flying all over the place, for something that's a bit of an edge-case.

But here we are...

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What do you think about comment #3 though?

Comment 6

[Graeme McCutcheon [:graememcc]]

> What do you think about comment #3 though?

I think I don't know enough about the painting code to answer that intelligently! I'll spend a bit of time this week having a look around, and collecting my thoughts.

For the moment:

Making that change, what does the NS_FRAME_SELECTED_CONTENT flag mean? At the moment it means a region of the frame is selected, and that the selection acting on it will be displayed. The comment 3 approach will make it mean that a region of the frame is selected, which may or may not be displayed. This seems less useful, and likely to catch people out in future

Code such as 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1888 
and possibly 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4049 
go from being simple flag tests to presumably having to compute/look up style rules.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 335301 [details] [diff] [review]
Patch v1

This seems reasonable. It actually seems kinda weird to pass in the range and the selection state without telling the frame what selection is actually involved.

The toolkit changes don't seem to be part of this patch, though, so you should resubmit a new patch for checkin without them.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it does make sense to change NS_FRAME_SELECTED_CONTENT to mean just a hint "something in this frame may be selected", but you're right, this isn't necessarily the right time to do that.

Comment 9

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v2 - now with less cross-bug pollution [Checkin: Comment 10]

> The toolkit changes don't seem to be part of this patch
And that's why you shouldn't work with patch queues when you're tired, lest you forget a "qpop" along the way. Apologies!

Carrying forward r+sr=roc

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 335566 [details] [diff] [review]
Patch v2 - now with less cross-bug pollution
[Checkin: Comment 10]

http://hg.mozilla.org/mozilla-central/rev/6cbd63d2a897

Comment 11

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080908091724 Minefield/3.1b1pre

I just filed bug 454790 for the fact that normal find doesn't highlight -moz-user-select: none nodes either. It seems to me that the fix for this bug makes it perhaps easier to fix that bug also.

Comment 12

[Martijn Wargers (dead)]

Sorry, but I now looked at the patch and I'm beginning to wonder, why adding a selectionType to setSelected methods?
Apparently, it isn't possible to do the IsSelectable() check in the nsTypedSelection::selectFrames function?

Comment 13

[Graeme McCutcheon [:graememcc]]

roc: correct me if i'm talking nonsense!

>Apparently, it isn't possible to do the IsSelectable() check in the
>nsTypedSelection::selectFrames function?

SetSelected recursively sets child frames, and a selectable frame can have a non-selectable child frame. Performing the IsSelectable() test outside of SetSelected, if the test decides that a frame is selectable, SetSelected could then select child frames that are not supposed to be selectable.

Comment 14

[Martijn Wargers (dead)]

Thanks for the explanation. Makes sense to me.