prasanthp96 Reporter
	Description • 7 years ago

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170809100326

Steps to reproduce:

Step 1: Go to https://en.wikipedia.org/
Step 2: In that page , click on the "English" link it will redirect the page to https://en.wikipedia.org/wiki/Main_Page and the Go back one page button gets activated
Step 3: Now click hamburger menu icon of firefox and choose Customize..
Step 4: Drag and Drop the sidebar icon to the left of the Go back one page button and click Done


Actual results:

The Go back one page button which was activated becomes inactive and it is not clickable.


Expected results:

The Go back one page button should remain active and one clicking it , the page should be redirected to the https://en.wikipedia.org/

	Madhuri
	Comment 1 • 7 years ago

Correct, 

The issue is reproducible when tested on - 

Win 10 X 64 and MacOS Sierra 10.12.5 

with Build ID 20170809100326 [57.0a1 (2017-08-09) (32-bit)]

	Alice0775 White
	Comment 2 • 7 years ago

[Tracking Requested - why for this release]:Session history is gone (Go/Back button state resets) after customize toolbar


Steps to reproduce:

Step 1: Navigate any webpage so that Go/Back button will be activated
Step 2: Enter customize mode and Exit customize mode
        --- observe the bug, Session history is gone (Go/Back button state resets)

	Alice0775 White
	Comment 3 • 7 years ago

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ecbeb9354c24bac41419cfbd71ce107f86ea4619&tochange=a91caf422971f0cf96ea62328fc318b44d07a2ca

Regressed by: Bug 1363485

	Dão Gottwald [:dao]
	Comment 4 • 7 years ago

Hmm, why are we wrapping items that aren't removable?

	:Gijs (he/him) Assignee
	Comment 5 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #4)
> Hmm, why are we wrapping items that aren't removable?

Everything gets wrapped in customize mode. The wrappers are also what gets an extra (transparent) border to make space for items that you drag inbetween the non-removable items.

The back/fwd buttons should be able to cope with being wrapped and unwrapped. It's not really clear to me why this was fine when they were in the urlbar container (which would also get wrapped) and isn't fine now.

	:Gijs (he/him) Assignee
	Comment 6 • 7 years ago

Context menus on those buttons, context menu on the page, and the history menu all still work, so I don't think this is dataloss, but I agree this is severe and will try to fix it ASAP. The problem is that the root issue is very old, in the DOM implementation of XUL, and never got fixed (or even particularly well-understood), and I am not sure how to either fix it or work around it yet.

	:Gijs (he/him) Assignee
	Comment 7 • 7 years ago

OK, so, most important questions answered after lots of logging in the depths of XUL broadcasting code...

Easy one first: why did this not happen before - because CustomizeMode wraps the outer container, not the inner buttons. As part of the wrapping, it removes the command/observes attributes to avoid disabled states changing while you're customizing. This didn't use to happen because items were in a toolbaritem node and so the broadcaster always stayed connected.

Harder one: why is this broken (ie what is the cause of bug 309953, at least today)?

When we navigate the browser, or switch tabs, we call UpdateBackForwardCommands. That adds/removes the disabled attribute from the broadcasters that update these buttons.

The XUL document keeps tabs on what broadcasters are hooked up to what listeners. This makes sense because the relationships go from listener to broadcaster, and it needs to know what listeners to update when the broadcaster updates.

This relationship is severed when:
1) the command attribute is removed.
2) the node gets added/removed from the DOM.

In addition to keeping tabs on the broadcaster<->listener relationships, it tracks which attributes need updating. There's only one problem with this... it's hard to update the lack of an attribute. That is, if I have a broadcaster with 1 attribute, a="foo", and I add a listener with b="bar", the broadcaster won't remove the b="bar" attribute.

This is what happens in our case:

1. back button is enabled. Neither broadcaster nor listener has a "disabled" attribute
2. we start customizing, both broadcaster and listener get a disabled attribute
3. we stop customizing
3a. the broadcaster gets updated. The disabled attribute gets removed. The back button is not updated because it's not currently hooked up to the broadcaster
3b. we change the command attribute on the back button back to what it normally is.
3c. we check if the command has a disabled attribute (it doesn't) and if it would have done, we would update the back button (we don't) to bring it in sync with the broadcaster


I can think of several fixes:

1) in https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/browser/components/customizableui/CustomizeMode.jsm#897-901 , add a clause that removes the disabled attribute if the button has it and the broadcaster doesn't.
Downside: this would break buttons that observe a broadcaster for something other than a disabled attribute, and set the disabled attribute independently.
Downside: we might hit this same issue again but with other attributes.

2) make the back/fwd button listen for customization end events and re-sync themselves with the broadcaster manually
downside: we will hit this again with other buttons and/or attributes.

3) keep track on any given broadcaster what attributes get removed without being re-added, and once an element starts observing a given broadcaster, remove said attributes
downside: lots of (C++, hashed sets, on DOM elements... yuck) work.
downside: Unclear if there will be users that don't expect this for some reason (hard to think why, we're effectively fixing a race between when the broadcaster+listener get created, but stranger things have happened).

4. put the back/fwd buttons back in a toolbaritem. They should be moved together anyway (they might become movable if we fix bug 1387313) and this will also fix this issue, at least for the back/fwd button.
downside: we will hit this again with other buttons and/or attributes.


Dão, does this make sense and do you have a preference on how we fix this?

	Dão Gottwald [:dao]
	Comment 8 • 7 years ago

(In reply to :Gijs from comment #7)
> OK, so, most important questions answered after lots of logging in the
> depths of XUL broadcasting code...
> 
> Easy one first: why did this not happen before - because CustomizeMode wraps
> the outer container, not the inner buttons. As part of the wrapping, it
> removes the command/observes attributes to avoid disabled states changing
> while you're customizing.

Can we let buttons opt out from this behavior? It shouldn't be possible for back/forward to change disabled states while customizing anyway.

	:Gijs (he/him) Assignee
	Comment 9 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #8)
> (In reply to :Gijs from comment #7)
> > OK, so, most important questions answered after lots of logging in the
> > depths of XUL broadcasting code...
> > 
> > Easy one first: why did this not happen before - because CustomizeMode wraps
> > the outer container, not the inner buttons. As part of the wrapping, it
> > removes the command/observes attributes to avoid disabled states changing
> > while you're customizing.
> 
> Can we let buttons opt out from this behavior? It shouldn't be possible for
> back/forward to change disabled states while customizing anyway.

We could, but while back/fwd are guaranteed to be disabled (I think?), many other buttons wouldn't be, so it wouldn't really help longterm. I think if we make a solution specifically for the back/fwd buttons, we might as well wrap them in a toolbaritem to begin with. Is there some reason not to do that that I'm missing?

	Dão Gottwald [:dao]
	Comment 10 • 7 years ago

My understanding is that this would have unwanted UX implications, see bug 1363485 comment 32.

	:Gijs (he/him) Assignee
	Comment 11 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #10)
> My understanding is that this would have unwanted UX implications, see bug
> 1363485 comment 32.

Oh! Hm, I missed that comment, thanks for pointing it out. OK, then adding some kind of specialized attribute is probably the simplest thing that could possibly work...

	Dão Gottwald [:dao]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8896569 [details]
Bug 1388990 - avoid breaking the broadcast/listener relationships of the back/fwd buttons when customizing,

https://reviewboard.mozilla.org/r/167836/#review173038

	Pulsebot
	Comment 14 • 7 years ago

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9d576871fd33
avoid breaking the broadcast/listener relationships of the back/fwd buttons when customizing, r=dao

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 15 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9d576871fd33

	u554753
	Comment 16 • 7 years ago

Verified on Windows, Mac, and Ubuntu.

	Madhuri
	Comment 17 • 7 years ago

[bugday-20170823] 

status-ff57.0a1 : VERIFIED & FIXED.

Managed to reproduce the issue on Firefox Nightly under Windows 10 X 64.

The issue is no more reproducible on Firefox latest Nightly [BuildID : 20170823100553 , 57.0a1 (2017-08-23) (32-bit)]

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 18 • 7 years ago

This is fixed, no need to track it.