Closed Bug 1345473 Opened 7 years ago Closed 7 years ago

Changing tab order using Compact themes cause an overlap

Categories

(Firefox :: Tabbed Browser, defect, P1)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox52 --- unaffected
firefox53 --- verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: ccomorasu, Assigned: jryans)

References

Details

Attachments

(2 files)

compact-themes-bug-1345473.gif
1.18 MB, image/gif
Details
Bug 1345473 - Set tab as drag source.
59 bytes, text/x-review-board-request
Gijs
: review+
gchang
: approval-mozilla-aurora+
gchang
: approval-mozilla-beta+
Details 
[Affected versions]:
 Fx 53.0b1
 Fx 54.0a2
 Fx 55.0a1

[Affected platforms]:
 Mac OS X 10.11.6
 Windows 7 x64
 Ubuntu 14.04 LTS

[Steps to reproduce]:
 1. Launch Firefox.
 2. Go to about:addons->Appearance and enable the "Compact Dark" theme.
 3. Open at least 3 tabs.
 4. Drag one of the tabs and drop it to the URL bar.

[Expected result]:
 The switch between tabs order goes accordingly.

[Actual result]:
 An overlap between the tabs occur.

[Regression range]:
 Will add the regression range in comments as soon as possible.

[Additional notes]:
 a. This occurs with the Compact Dark theme from about:support.
 b. Screen shot with the issue: http://imgur.com/a/JLWhE .
[Regression Range]:

 Last good revision: d8f63b2935af0915a6a24b3ea8e27d9a09f66416 (2016-12-09)
 First bad revision: 8404d26166a35406f46ff237ed132735c98882b2 (2016-12-10)
 Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d8f63b2935af0915a6a24b3ea8e27d9a09f66416&tochange=8404d26166a35406f46ff237ed132735c98882b2
Has Regression Range: --- → yes
Has STR: --- → yes
I can reproduce this easily, see attached. I think this is pretty bad, it breaks tab dragging completely for the current window ( opening a new window  ).
Priority: -- → P1
Assignee: nobody → jryans
Status: NEW → ASSIGNED
(In reply to Cristian Comorasu [:CristiComo] from comment #1)
> [Regression Range]:
> 
>  Last good revision: d8f63b2935af0915a6a24b3ea8e27d9a09f66416 (2016-12-09)
>  First bad revision: 8404d26166a35406f46ff237ed132735c98882b2 (2016-12-10)
>  Pushlog:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=d8f63b2935af0915a6a24b3ea8e27d9a09f66416&tochange=8404
> d26166a35406f46ff237ed132735c98882b2

I am not sure about this regression range, because Compact Themes did not land until 2017-01-14 (bug 1314091).  Does this mean you saw this problem even without Compact Themes?
Flags: needinfo?(cristian.comorasu)
It appears that when Compact Themes are enabled, `_finishAnimateTabMove` is not called if you drop the tab on the URL bar.  With the default theme, it is called, via the `dragend` handler on #tabbrowser-tabs.

After some style bisection, it appears only the following rules are needed in compacttheme.css to trigger the problem:

.tab-background {
  visibility: hidden;
}

.tabbrowser-tab {
  /* We normally rely on other tab elements for pointer events, but this
     theme hides those so we need it set here instead */
  pointer-events: auto;
}

If you use the Style Editor to replace compacttheme.css with just those rules, you still have the bug.  If you delete them both, the bug goes away.
Gijs, do you have any theories on why the above styles (see comment 4) would prevent dragend events from reaching #tabbrowser-tabs?  If not, I can try to look deeper, but I'd thought it was worth asking first.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Gijs, do you have any theories on why the above styles (see comment 4) would
> prevent dragend events from reaching #tabbrowser-tabs?  If not, I can try to
> look deeper, but I'd thought it was worth asking first.

Is something calling preventDefault for drag events on the individual tabs (which have pointer-events: auto per one of those styles)?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> (In reply to Cristian Comorasu [:CristiComo] from comment #1)
> > [Regression Range]:
> > 
> >  Last good revision: d8f63b2935af0915a6a24b3ea8e27d9a09f66416 (2016-12-09)
> >  First bad revision: 8404d26166a35406f46ff237ed132735c98882b2 (2016-12-10)
> >  Pushlog:
> > https://hg.mozilla.org/mozilla-central/
> > pushloghtml?fromchange=d8f63b2935af0915a6a24b3ea8e27d9a09f66416&tochange=8404
> > d26166a35406f46ff237ed132735c98882b2
> 
> I am not sure about this regression range, because Compact Themes did not
> land until 2017-01-14 (bug 1314091).  Does this mean you saw this problem
> even without Compact Themes?

This was reproducible also with DevEdition theme installed (https://addons.mozilla.org/en-Us/firefox/addon/devedition-theme-enabler/), this is how we managed to get that regression, altough a bit harder.
To note that using the default theme the issues is not reproducible.
Flags: needinfo?(cristian.comorasu)
(In reply to :Gijs from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > Gijs, do you have any theories on why the above styles (see comment 4) would
> > prevent dragend events from reaching #tabbrowser-tabs?  If not, I can try to
> > look deeper, but I'd thought it was worth asking first.
> 
> Is something calling preventDefault for drag events on the individual tabs
> (which have pointer-events: auto per one of those styles)?

So far, I'm not seeing `preventDefault` or `stopPropagation` that would apply to this case...  I tried adding a `dragend` handler directly on the tab element, and it fires with the Default theme but not with Compact when dropped on to the URL bar.

At the moment, not sure how to proceed here.
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> (In reply to :Gijs from comment #6)
> > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > > Gijs, do you have any theories on why the above styles (see comment 4) would
> > > prevent dragend events from reaching #tabbrowser-tabs?  If not, I can try to
> > > look deeper, but I'd thought it was worth asking first.
> > 
> > Is something calling preventDefault for drag events on the individual tabs
> > (which have pointer-events: auto per one of those styles)?
> 
> So far, I'm not seeing `preventDefault` or `stopPropagation` that would
> apply to this case...  I tried adding a `dragend` handler directly on the
> tab element, and it fires with the Default theme but not with Compact when
> dropped on to the URL bar.
> 
> At the moment, not sure how to proceed here.

Neil, can you help? I'm a bit lost at what's going on here.
Flags: needinfo?(enndeakin)
I'm unable to reproduce this on Linux.
If this doesn't happen on Linux at all (counter to comment #0) and the CSS in comment #4, my other suspicion would be something to do with hit testing and bug 1227562, but given that comment #0 does mention Linux and the CSS in comment #4 is apparently necessary and sufficient, I don't see how it could be that. Flagging it up anyway in case some of my assumptions are broken, given comment #10.
Here's my testing results by OS:

Bad
---

Nightly 55 on macOS 10.12.3

Good
----

Nightly 55 (2017-03-07) on Windows 10
Nightly 55 on Ubuntu 16.04

(I tried to run today's Nightly 55 on Windows 10, but it crashed immediately on startup, so I went back a few days to avoid it.  Looks like it's a known issue filed as bug 1346215.)

Cristian, can you confirm if this agrees with your experience?  Is it actually a macOS only issue?
Flags: needinfo?(cristian.comorasu)
I can't reproduce this on Mac. The dragend event is fired at nsIDragSession::sourceNode, can you check what that is assigned to?
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #13)
> I can't reproduce this on Mac. The dragend event is fired at
> nsIDragSession::sourceNode, can you check what that is assigned to?

Okay, I've learned a few more details.  The bug only happens if you begin the drag using the tab's title text node within the tab element (`sourceNode` is the text node).

If you instead start dragging from the edges of the tab element outside of the text node, then the `sourceNode` is something else, such as the .tab-label, and the `dragend` event gets through.

I have verified that `nsBaseDragService::FireDragEventAtSource` is called in both the good and bad case to fire a `dragend` event, and it passes it along to the `presShell` for handling.

So it seems in the bad case, something is preventing the `dragend` event from bubbling up from the tab's title text node, while other elements remain working as expected.

Neil, does this help you reproduce?  Or if not, what else should I investigate here?
Flags: needinfo?(enndeakin)
Hello Ryan!
Some of my colleagues had the crash with the startup, it was an issue which is fixed now(see bug 1346215).
However the issue is still reproducible with the latest Nightly(see http://imgur.com/a/4e6G9).
Cheers!
Flags: needinfo?(cristian.comorasu)
I can't reproduce it, but this is probably just 460801.
Flags: needinfo?(enndeakin)
(In reply to Cristian Comorasu [:CristiComo] from comment #15)
> Hello Ryan!
> Some of my colleagues had the crash with the startup, it was an issue which
> is fixed now(see bug 1346215).
> However the issue is still reproducible with the latest Nightly(see
> http://imgur.com/a/4e6G9).
> Cheers!

Hmm, interesting.  This screenshot looks like Windows 7 Aero, but I can't seem to trigger the issue there myself.

I suppose it's good to know that it's probably not platform specific, at least.
(In reply to Neil Deakin from comment #16)
> I can't reproduce it, but this is probably just 460801.

I could be wrong, but the source node doesn't appear to be moved / removed from the DOM when the bug triggers.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> (In reply to Neil Deakin from comment #16)
> > I can't reproduce it, but this is probably just 460801.
> 
> I could be wrong, but the source node doesn't appear to be moved / removed
> from the DOM when the bug triggers.

Do we trigger the creation/destruction of pseudo-elements when the tab is hovered, maybe?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> (In reply to Neil Deakin from comment #16)
> > I can't reproduce it, but this is probably just 460801.
> 
> I could be wrong, but the source node doesn't appear to be moved / removed
> from the DOM when the bug triggers.

Hmm, well, now I am not so sure anymore...  I've added more logging at the point where the `dragend` event is sent for dispatch.  

* When the source node is the tab's title #text node and you drop it on the URL bar, the #text node is disconnected and seems to have no parents, so the event basically goes nowhere
* When the source node is the tab's title #text node and you drop within the other tabs, the #text node has xul:label.tab-label as its parent, so the event reaches the expected handlers

I am still a bit lost as to _why_ that happens.  In any case, it appears we can work around the issue here by disabling pointer events on the .tab-label with Compact Themes.

I wish I understand the root cause more fully...
Better would be to call dataTransfer.addElement(tab) during dragstart so the tab is consistently the element being dragged.
(In reply to Neil Deakin from comment #21)
> Better would be to call dataTransfer.addElement(tab) during dragstart so the
> tab is consistently the element being dragged.

Thanks Neil!  That also seems to work, so let's go with that, since it's less mysterious.
:CristiComo, once builds are available for the try run below, can you please double check that this appears to resolve the issue for you as well?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d0c2f8e2d9803e2287a4d09d9bcf59b81b3a320
Flags: needinfo?(cristian.comorasu)
Hi Ryan!
I could not reproduce this issue using the builds from comment 24.
Flags: needinfo?(cristian.comorasu)
Comment on attachment 8846843 [details]
Bug 1345473 - Set tab as drag source.

https://reviewboard.mozilla.org/r/119838/#review122144

Thanks for tracking this down!
Attachment #8846843 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/da02f041b545
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8846843 [details]
Bug 1345473 - Set tab as drag source.

Approval Request Comment
[Feature/Bug causing the regression]: Somehow triggered by Compact Themes which shipped in 53
[User impact if declined]: If declined, the tab bar can become locked in a broken state with Compact Themes.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Verified locally and on a try build by QE
[Needs manual test from QE? If yes, steps to reproduce]: No, tested in try already
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only changes how the element used for tab dragend events is determined
[String changes made/needed]: None
Attachment #8846843 - Flags: approval-mozilla-beta?
Attachment #8846843 - Flags: approval-mozilla-aurora?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #29).
> [Is this code covered by automated tests?]: No

Actually, there probably are existing automated tests in this area...?  I'm not sure, I don't usually work on the tab bar.  Anyway, we did not add additional tests at this time.
Comment on attachment 8846843 [details]
Bug 1345473 - Set tab as drag source.

Fix an UI issue related to compact themes. Aurora54+ & Beta53+.
Attachment #8846843 - Flags: approval-mozilla-beta?
Attachment #8846843 - Flags: approval-mozilla-beta+
Attachment #8846843 - Flags: approval-mozilla-aurora?
Attachment #8846843 - Flags: approval-mozilla-aurora+
I rechecked with the latest nightly (20170315030215) on Windows 10 x64 and Mac OS X 10.12.3, I can confirm the issue is fixed.
status-firefox55: fixed → verified
Let's make sure this works as intended on 53 as well.
Flags: qe-verify+
I have reproduced the issue mentioned in comment 0, using an affected Firefox 53.0b1 build. 

I have verified that the issue is not reproducible using Firefox 53.0b5 (Build Id:20170320143328) and Firefox 54.0a2 (Build Id: 20170323004002) on Windows 7 64bit, Mac Os X 10.11.6 and Ubuntu 14.04 32 bit.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: