307256 - [FIX]drag and drop reordering of tabs broken

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect, P1)

Description

[u88484]

Noticed this with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1)
Gecko/20050906 Firefox/1.6a1

Hopefully someone else can track down a regression date...I don't have time for
at least another day.

Comment 1

[Jesse Ruderman]

WFM Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4)
Gecko/20050906 Firefox/1.4

Comment 2

[Jesse Ruderman]

WFM Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1)
Gecko/20050905 Firefox/1.6a1

Comment 3

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050906
Firefox/1.4 ID:2005090612

WFM

Comment 4

[u88484]

Cofirmed works on branch but on trunk I tracked this down to regressing between
2005090507 and 2005090607 builds.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050906
Firefox/1.6a1 ID:2005090607

Comment 5

[Jesse Ruderman]

Confirmed Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1)
Gecko/20050906 Firefox/1.6a1.

So this regressed between Sept 5 and Sept 6 builds, and only on trunk.

Comment 6

[u88484]

ya I will try and check out between which hourlys.

Comment 7

[Peter van der Woude [:Peter6]]

maybe Bug 241503 , which should land on branch next week

Comment 8

[TimoT]

I don't think that this was caused by bug 241503, backed it out locally and this
is still broken

Comment 9

[TimoT]

This was caused/exposed by bug 306262

Comment 10

[Boris Zbarsky [:bzbarsky]]

So what are the steps to reproduce, exactly?  I don't see any in the bug...

Is there actually any visible selection involved?

Comment 11

[TimoT]

To reproduce:

1) Open 2 tabs
2) Try to drag either of the tabs

The tabs behave like they did before the tab drag & drop code landed (dragging
only gives focus to the tab you click first)

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  So I can fix this by making tabbox have "-moz-user-select: none".  But
perhaps it would be better to set that for xul|* in xul.css the way we set
-moz-user-focus?  Neil, mconnor, jag, what do you think?

Comment 13

[PikeUK]

*** Bug 307326 has been marked as a duplicate of this bug. ***

Comment 14

[u88484]

*** Bug 307342 has been marked as a duplicate of this bug. ***

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Based on bug 307304, I think this is what we want... Otherwise random XUL nodes
(eg tabs) can clobber the selection when you just click on them.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Given that bug 306262 blocks 1.8b5, this one should too.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195221 [details] [diff] [review]
Proposed patch

This breaks selection in XUL textboxes.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: More extensive fix

Oh, for some sanity in -moz-user-select!  This seems to make things happy for
me.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195300 [details] [diff] [review]
More extensive fix

Mike, could you look at this too, please?

Comment 20

[Martijn Wargers (dead)]

That patch would break selection of marquee text, wouldn't it? (because marquee
uses xul:hbox)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix for marquee too

Good catch, Martijn!

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 195300 [details] [diff] [review]
More extensive fix

This includes a variant of the patch in bug 203291.  I don't like having 'all'
and '-moz-all' or 'text' and '-moz-text', but I guess file a followup bug on
that.  (The names should be standardizable with the -moz- removed.)

But the comment in nsIFrame::IsSelectable disagrees with the code and with the
comments in 
nsStyleConsts.h.  What's supposed to happen, and why don't -moz-text and
-moz-all look more similar in the code or more different in the comments?

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 195429 [details] [diff] [review]
Fix for marquee too

This looks unneeded now that bug 277208 has landed, and certainly doesn't apply
anymore.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195429 [details] [diff] [review]
Fix for marquee too

It's still needed on branch, where all this should really land...

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 195429 [details] [diff] [review]
Fix for marquee too

ok, r+sr=dbaron for 1.8 branch

Comment 26

[Boris Zbarsky [:bzbarsky]]

> What's supposed to happen, and why don't -moz-text and
> -moz-all look more similar in the code or more different in the comments?

As currently implented, -moz-all on a node forces selection on all descendants
unless an ancestor node has "none" set.  My implementation of -moz-text forces
selection of text on descendants no matter what's set on ancestors.  I'd be
happy to clarify the comments accordingly.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 195300 [details] [diff] [review]
More extensive fix

ok, yeah, fix the comments, and perhaps try to think of a better name...

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195300 [details] [diff] [review]
More extensive fix

This doesn't fix bug 307924  :(

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Better fix

OK, so what this does is introduce a -moz-none which, unlike "none", doesn't
clobber the descendants' -moz-user-select values.  Then use this for XUL and
explicitly style HTML inputs and textareas as "text" to make sure that things
inside them are selectable.  The reason the first patch didn't work is that
some textboxes actually have random stuff inside (created by the binding) that
really isn't supposed to be selectable... and if it's selectable can't be
dragged.

Comment 30

[Simon Paquet [:sipaq]]

Unfortunately, the patch doesn't fix bug 307411.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Did any of the patches in this bug ever do that?  It wasn't clear to me that
backing out the patch from bug 306262 fixed it either (if it does, no one
mentioned that in that bug...)

Comment 32

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195429 [details] [diff] [review]
Fix for marquee too

Requesting 1.8b5 approval so we can land bug 306262 too.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195648 [details] [diff] [review]
Better fix

Requesting 1.8b5 approval so we can land bug 306262 too.

Comment 35

[Simon Paquet [:sipaq]]

(In reply to comment #30)
> Unfortunately, the patch doesn't fix bug 307411.

Boris, I suck. It seems that I didn't apply your patch correctly. This patch
does indeed fix bug 307411. Sorry for the confusion.

Marking VERIFIED FIXED Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050911 Firefox/1.6a1.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Asa, is attachment 195429 [details] [diff] [review] ok for branch too?  I can't really land the other
patch here or the one in bug 306262 without it.

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 195429 [details] [diff] [review]
Fix for marquee too

absolutely. sorry for not marking that.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.