Last Comment Bug 451252 - "Highlight All" find selection not visible in elements that have -moz-user-select: none set
: "Highlight All" find selection not visible in elements that have -moz-user-se...
Status: VERIFIED FIXED
: testcase
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla1.9.1b1
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
Depends on:
Blocks: 263683
  Show dependency treegraph
 
Reported: 2008-08-19 13:55 PDT by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2008-09-15 07:26 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (300 bytes, text/html)
2008-08-19 13:55 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Patch v1 (34.27 KB, patch)
2008-08-24 14:39 PDT, Graeme McCutcheon [:graememcc]
roc: review+
roc: superreview+
Details | Diff | Review
Patch v2 - now with less cross-bug pollution [Checkin: Comment 10] (22.65 KB, patch)
2008-08-26 11:52 PDT, Graeme McCutcheon [:graememcc]
graeme.mccutcheon: review+
graeme.mccutcheon: superreview+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2008-08-19 13:55:33 PDT
Created attachment 334555 [details]
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] 2008-08-20 13:20:13 PDT
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) (Exited; email my personal email if necessary) 2008-08-20 15:22:35 PDT
That sounds right. Is it really yuck?
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-20 15:24:27 PDT
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] 2008-08-24 14:39:29 PDT
Created attachment 335301 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2008-08-24 15:50:43 PDT
What do you think about comment #3 though?
Comment 6 Graeme McCutcheon [:graememcc] 2008-08-25 13:11:29 PDT
> 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) (Exited; email my personal email if necessary) 2008-08-25 14:37:43 PDT
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) (Exited; email my personal email if necessary) 2008-08-25 14:39:35 PDT
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] 2008-08-26 11:52:51 PDT
Created attachment 335566 [details] [diff] [review]
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) 2008-09-07 07:14:35 PDT
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 [:mwargers] (gone per 2016-05-31 :-( ) 2008-09-11 06:42:51 PDT
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 [:mwargers] (gone per 2016-05-31 :-( ) 2008-09-11 17:44:07 PDT
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] 2008-09-12 13:06:34 PDT
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 [:mwargers] (gone per 2016-05-31 :-( ) 2008-09-15 07:26:45 PDT
Thanks for the explanation. Makes sense to me.

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