Closed Bug 307256 Opened 19 years ago Closed 19 years ago

[FIX]drag and drop reordering of tabs broken

Categories

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

Product:
Core
Component:
DOM: Selection
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8beta4

People

(Reporter: u88484, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.8, regression, Whiteboard: trunk only)

Attachments

(2 files, 2 obsolete files)

Fix for marquee too
974 bytes, patch
dbaron
: review+
dbaron
: superreview+
asa
: approval1.8b5+
Details | Diff | Splinter Review
Better fix
8.75 KB, patch
dbaron
: review+
dbaron
: superreview+
asa
: approval1.8b5+
Details | Diff | Splinter Review
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.
Flags: blocking1.9a1?
WFM Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050906 Firefox/1.4
WFM Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050905 Firefox/1.6a1
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050906 Firefox/1.4 ID:2005090612 WFM
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
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.
ya I will try and check out between which hourlys.
maybe Bug 241503 , which should land on branch next week
I don't think that this was caused by bug 241503, backed it out locally and this is still broken
This was caused/exposed by bug 306262
So what are the steps to reproduce, exactly? I don't see any in the bug... Is there actually any visible selection involved?
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)
Blocks: 306262
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?
*** Bug 307326 has been marked as a duplicate of this bug. ***
*** Bug 307342 has been marked as a duplicate of this bug. ***
Whiteboard: trunk only
Blocks: 307304
Assignee: nobody → selection
Component: Tabbed Browser → Selection
Product: Firefox → Core
QA Contact: tabbed.browser
Attached patch Proposed patch (obsolete) — — Splinter Review
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.
Assignee: selection → bzbarsky
Status: NEW → ASSIGNED
Attachment #195221 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #195221 - Flags: review?(mconnor)
Given that bug 306262 blocks 1.8b5, this one should too.
Flags: blocking1.9a1? → blocking1.8b5?
Priority: -- → P1
Summary: drag and drop reordering of tabs broken → [FIX]drag and drop reordering of tabs broken
Target Milestone: --- → mozilla1.8beta4
Comment on attachment 195221 [details] [diff] [review] Proposed patch This breaks selection in XUL textboxes.
Attachment #195221 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #195221 - Flags: superreview-
Attachment #195221 - Flags: review?(mconnor)
Attachment #195221 - Flags: review-
Blocks: 307198
Blocks: 307411
Attached patch More extensive fix (obsolete) — — Splinter Review
Oh, for some sanity in -moz-user-select! This seems to make things happy for me.
Attachment #195221 - Attachment is obsolete: true
Attachment #195300 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #195300 - Flags: review?(dbaron)
Comment on attachment 195300 [details] [diff] [review] More extensive fix Mike, could you look at this too, please?
Attachment #195300 - Flags: review?(mconnor)
That patch would break selection of marquee text, wouldn't it? (because marquee uses xul:hbox)
Attachment #195300 - Flags: review?(mconnor) → review+
Good catch, Martijn!
Attachment #195429 - Flags: superreview?(dbaron)
Attachment #195429 - Flags: review?(dbaron)
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 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.
Attachment #195429 - Flags: superreview?(dbaron)
Attachment #195429 - Flags: superreview-
Attachment #195429 - Flags: review?(dbaron)
Attachment #195429 - Flags: review-
Comment on attachment 195429 [details] [diff] [review] Fix for marquee too It's still needed on branch, where all this should really land...
Attachment #195429 - Flags: superreview?(dbaron)
Attachment #195429 - Flags: superreview-
Attachment #195429 - Flags: review?(dbaron)
Attachment #195429 - Flags: review-
Comment on attachment 195429 [details] [diff] [review] Fix for marquee too ok, r+sr=dbaron for 1.8 branch
Attachment #195429 - Flags: superreview?(dbaron)
Attachment #195429 - Flags: superreview+
Attachment #195429 - Flags: review?(dbaron)
Attachment #195429 - Flags: review+
> 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 on attachment 195300 [details] [diff] [review] More extensive fix ok, yeah, fix the comments, and perhaps try to think of a better name...
Attachment #195300 - Flags: review?(dbaron) → review+
Blocks: 307924
Comment on attachment 195300 [details] [diff] [review] More extensive fix This doesn't fix bug 307924 :(
Attachment #195300 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attached patch Better fix — — Splinter Review
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.
Attachment #195300 - Attachment is obsolete: true
Attachment #195648 - Flags: superreview?(dbaron)
Attachment #195648 - Flags: review?(dbaron)
Attachment #195648 - Flags: superreview?(dbaron)
Attachment #195648 - Flags: superreview+
Attachment #195648 - Flags: review?(dbaron)
Attachment #195648 - Flags: review+
Unfortunately, the patch doesn't fix bug 307411.
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...)
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment on attachment 195429 [details] [diff] [review] Fix for marquee too Requesting 1.8b5 approval so we can land bug 306262 too.
Attachment #195429 - Flags: approval1.8b5?
Comment on attachment 195648 [details] [diff] [review] Better fix Requesting 1.8b5 approval so we can land bug 306262 too.
Attachment #195648 - Flags: approval1.8b5?
(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.
Status: RESOLVED → VERIFIED
Attachment #195648 - Flags: approval1.8b5? → approval1.8b5+
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 on attachment 195429 [details] [diff] [review] Fix for marquee too absolutely. sorry for not marking that.
Attachment #195429 - Flags: approval1.8b5? → approval1.8b5+
Flags: blocking1.8b5? → blocking1.8b5+
Fixed on 1.8 branch.
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: