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.
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?
That sounds right. Is it really yuck?
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.
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...
What do you think about comment #3 though?
> 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 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.
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.
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 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
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.
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?
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.
Thanks for the explanation. Makes sense to me.