Mihai Boldan, Desktop QA [:mboldan] Reporter
	Description • 7 years ago

[Affected versions]:
- Firefox 54.0a1 (2017-02-27)

[Affected platforms]:
- Mac OS X 10.11.6, Windows 10x64, Ubuntu 16.04x64

[Steps to reproduce]:
1. Launch Firefox.
2. Open Toggle Tools.
3. Enable Toggle rulers for the page.
4. Refresh the page.

[Expected result]:
- Toggle rulers for the page tool is disabled. 

[Actual result]:
- Toggle rulers for the page tool is disabled, but the Rulers button is still enabled. 

[Regression range]:
Last good revision: 83e1a88a9833a498a0e841e37a822de99b0ed0c6
First bad revision: f6895ed1367c698cd60af71084f68950cdb2e630
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=83e1a88a9833a498a0e841e37a822de99b0ed0c6&tochange=f6895ed1367c698cd60af71084f68950cdb2e630

It seems that Bug 1320149 caused this issue.

[Additional notes]:
- This issue is also reproducible with 'Measure a portion of the page' tool.
- If clicking on the Ruler button after step 4, the tool is enabled and the button is disabled.(See https://i.imgur.com/MSvW5fb.png )

	Patrick Brosset <:pbro>
	Comment 1 • 7 years ago

Inspector bug triage (filter on CLIMBING SHOES).

	Matteo Ferretti [:zer0] [:matteo] Assignee
	Comment 3 • 7 years ago

Comment on attachment 8848050 [details]
Bug 1342928 - Keep the commands / buttons state in sync;

Asking also for feedback to both ochameau and jryans since it's related to both bug 1320149 and bug 1341756.

A couple of note:

1. About "changed" event:
Every time we change the state of a command, we emit a "changed" event with an object with both `target` and `command` as event object; so an observer could potentially use those properties to filter which command / target needs to process.
However, the current state of the command's `definition` provides directly a `onChange` function reference that will be executed, without any control to the event object given. It means, if we have both "rulers" and "measure" commands state registered, and only "rulers" state change, both of them will be notified that something changed, and both of them will queries `isEnabledForTarget` (since only "rulers" was enabled, we don't have inconsistency with that).

To avoid that we could either make `definition` more complex (with storing generated function somewhere) or having a runtime generated name event ("rulers-changed" for example).

I personally think that the additional complexity or inelegance of the latter is not worth, and I prefer the current code, that is much cleaner. However, I'd like to have other opinions about that.

2. About RDM's swap.js:
This is an inconsistency that has nothing to do with bug 1320149, and only partially related with bug 1341756.
Basically, when we enter / exit in / from RDM, the highlighter stops to work. The markup was still visible before bug 1341756, that fixes that.
Since entering in RDM keeps the same window's id (both outer and inner), the button's state doesn't change. It means, if we enable the "rulers" for instance and then we enter in RDM, the rulers will be destroyed, but the button will be still checked.
I didn't find an easy way to fix that, so after talking with Alex we ended up to add the workaround in `swap.js`. If there is a better place or way to do so, I would love to hear.

I would also like to figure out how to make the highlighters working across the bfcache / swap, but it would require more time and investigation, and I think is also unrelated to this patch. So I tried to find an approach to keep the state consistent.

	Joe Walker [:jwalker]
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8848050 [details]
Bug 1342928 - Keep the commands / buttons state in sync;

https://reviewboard.mozilla.org/r/121022/#review122974

::: devtools/client/responsive.html/browser/swap.js:147
(Diff revision 1)
>        gBrowser.updateCurrentBrowser(true);
>  
> +      // Since bug 1341756 when we're entering in RDM we're clearing any highlighters;
> +      // so we have to update the command's state too.
> +      CommandState.disableForTarget({tab}, "measure");
> +      CommandState.disableForTarget({tab}, "rulers");

I'm concerned that we'll forget to update this list when we alter the commands. Isn't there a way to loop over the definitions and call disableForTarget everywhere that's needed?

	Joe Walker [:jwalker]
	Comment 5 • 7 years ago

r+ from me, but I think Alex or JRyans should give it an additional r+ please.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8848050 [details]
Bug 1342928 - Keep the commands / buttons state in sync;

https://reviewboard.mozilla.org/r/121022/#review123030

Would it be possible for these commands to send an event back to the client when they are disabled, so we can use that to keep the UI updated?

That would avoid having to mark up places like RDM with a list of things that silently disappear.  I'm also concerned like Joe about forgetting the fixed list in RDM.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 7 • 7 years ago

Comment on attachment 8848050 [details]
Bug 1342928 - Keep the commands / buttons state in sync;

f- for now, but let me know if my suggestion is too complex.

	Alexandre Poirot [:ochameau]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8848050 [details]
Bug 1342928 - Keep the commands / buttons state in sync;

https://reviewboard.mozilla.org/r/121022/#review123392

::: devtools/client/responsive.html/browser/swap.js:147
(Diff revision 1)
>        gBrowser.updateCurrentBrowser(true);
>  
> +      // Since bug 1341756 when we're entering in RDM we're clearing any highlighters;
> +      // so we have to update the command's state too.
> +      CommandState.disableForTarget({tab}, "measure");
> +      CommandState.disableForTarget({tab}, "rulers");

You should keep that for a followup.
This is unrelated to bug 1320149's regression
and appears to be controversial.
The rest of the patch is already complex enough to focus on that first.

	Alexandre Poirot [:ochameau]
	Comment 9 • 7 years ago

That hack in swap.js is more related to bug 1341756.
It relates to something around RDM breaking the highlighters, this is the reason I suggested to hack it in rdm.
But yes, ideally the highlighters would be fixed to support RDM toggle sanely, or at least update its state sanely.

	Matteo Ferretti [:zer0] [:matteo] Assignee
	Comment 10 • 7 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)

> Would it be possible for these commands to send an event back to the client
> when they are disabled, so we can use that to keep the UI updated?

That's exactly what happened. The reason because I have to add those in RDM, is because the point 2. Since swapping destroy the anonymous content, the UI is not getting updated since there is nothing that notify the command's state. The commands are not getting disabled, since nothing disable them, it's just that on "server" (content side) the markup of highlighters are removed by the swapping of RDM.

Notice that this issue is not strictly related to this patch – Alex in fact suggested to filed a new bug – but I wanted at least have the state consistent when landing this.
Basically the RDM was breaking the state of those buttons even before Bug 1320149.

Other things that I tried:
1. Verifying if swapping frames was actually changed the window's innerID – it's not; if it was the case I could actually fix that more easily.

2. trying to listen to `window-ready` in the command itself – but the `target`, even if it's a tabActor, apparently don't dispatch this event, after a bit of investigation I decided to go for another approach since also Alex didn't want put this logic in the commands themselves.

3. Trying to communicate from the "server" command to the "client" command; but I didn't find an elegant way to do so (maybe Joe could you have some thoughts about that).

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #4)

> I'm concerned that we'll forget to update this list when we alter the
> commands. Isn't there a way to loop over the definitions and call
> disableForTarget everywhere that's needed?

We could do something like that, but the problem is marks what is needed and what is not. For example, I could easily loop over all the buttons that are managed in the CommandState and disable them, however paintflashing doesn't need to be disabled. So we could either:

1. Add a method / parameter to mark the commands that needs to be disabled in such situation, when we enable them (I don't know how I would call this method / parameter, honestly)

2. Loop over everything and just disable everything no matter what.
Since for the moment the only exception is paintflashing, I think we could probably live with that. It would also makes the UX more consistent (avoiding "why those tools are disabled when I enter in RDM and this is not?")

As said, in an ideal world I would love to make it working instead of hide / disable everything, and I'm investigate on that, but it would take more time.

	Matteo Ferretti [:zer0] [:matteo] Assignee
	Comment 11 • 7 years ago

I'd like to have your opinions about how we should proceed.

If you all agreed, I would disable by default all the commands in this patch; so we don't have to keep updated the list – as explained in my last comment.

In that way we can have at least the toolbox's buttons in the proper state when dealing with RDM.

I can file a follow up bug (to either figure out how to make the highlighters working in such situation, or figure out how the "server" command could send a notification to the "client" part), and mention that in a comment in swap.js before we disable all the commands.

It sounds reasonable to you?

As alternative, I can just remove that hack from swap.js and land the rest of the patch; but that it means that we'll have the commands' state not updated when we enter/exit in/from RDM, and I'd rather have a temporary fix to than no fix at all until we'll figure out the proper fix.

	Alexandre Poirot [:ochameau]
	Comment 12 • 7 years ago

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #11)
> As alternative, I can just remove that hack from swap.js and land the rest
> of the patch; but that it means that we'll have the commands' state not
> updated when we enter/exit in/from RDM, and I'd rather have a temporary fix
> to than no fix at all until we'll figure out the proper fix.

As said in my previous comment, I would do that.
This RDM issue is unrelated to this bug report.
This isn't a followup, that is yet another regression more related to bug 1341756 than bug 1320149.

	Matteo Ferretti [:zer0] [:matteo] Assignee
	Comment 13 • 7 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> (In reply to Matteo Ferretti [:zer0] [:matteo] from comment #11)
> > As alternative, I can just remove that hack from swap.js and land the rest
> > of the patch; but that it means that we'll have the commands' state not
> > updated when we enter/exit in/from RDM, and I'd rather have a temporary fix
> > to than no fix at all until we'll figure out the proper fix.

> As said in my previous comment, I would do that.
> This RDM issue is unrelated to this bug report.
> This isn't a followup, that is yet another regression more related to bug
> 1341756 than bug 1320149.

Agreed that is not related to bug 1320149, but is not related to bug 1341756 either.

I would like to land a temporary fix in this patch, for the user, in the meantime we figured out a better fix, and explain that in the comment, but if this is only me and everyone of you guys will prefer do not hack swap.js and leave the bug when we enter / exit in RDM until we properly fix it let me know.
The problem is, I don't know how much it will take to fix it yet. That's why I'm inclined to fix it now with this "hack".

	Joe Walker [:jwalker]
	Comment 14 • 7 years ago

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #10)

> (In reply to Joe Walker [:jwalker] from comment #4)
> 
> > I'm concerned that we'll forget to update this list when we alter the
> > commands. Isn't there a way to loop over the definitions and call
> > disableForTarget everywhere that's needed?
> 
> We could do something like that, but the problem is marks what is needed and
> what is not. For example, I could easily loop over all the buttons that are
> managed in the CommandState and disable them, however paintflashing doesn't
> need to be disabled. So we could either:

I've lost track of why we don't just make the toolbox correctly represent the state of the tools, which means we shouldn't need to be blanket turning them on or off.

Also we're taking up lots of people's time with review and feedback on this bug. Please could you pick just one reviewer (not me).

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 17 • 7 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> (In reply to Matteo Ferretti [:zer0] [:matteo] from comment #11)
> > As alternative, I can just remove that hack from swap.js and land the rest
> > of the patch; but that it means that we'll have the commands' state not
> > updated when we enter/exit in/from RDM, and I'd rather have a temporary fix
> > to than no fix at all until we'll figure out the proper fix.
> 
> As said in my previous comment, I would do that.
> This RDM issue is unrelated to this bug report.
> This isn't a followup, that is yet another regression more related to bug
> 1341756 than bug 1320149.

Yes, let's leave RDM out of this for now, and we can look into an approach for that separately.

I think that means :zer0's latest patch revision is ready to land.

Thanks for looking into this issue :zer0!

	Matteo Ferretti [:zer0] [:matteo] Assignee
	Comment 19 • 7 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17)

> I think that means :zer0's latest patch revision is ready to land.

Yes, I fixed some issues in the tests and as soon as the last try build is over I will land this commit:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17f0605ab796a19e39cbb3e4647e83936eb12738&selectedJob=85287698

It seems doing well so far.

I filed bug 1349229 about the broken state given by toggling the RDM.

	Matteo Ferretti [:zer0] [:matteo] Assignee
	Comment 20 • 7 years ago

They've stopped to 98% since a while, but the related tests are done; so I'm proceeding to land this patch.

	Pulsebot
	Comment 21 • 7 years ago

Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ea9ef8b5fb6
Keep the commands / buttons state in sync; r=jwalker

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2ea9ef8b5fb6

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 7 years ago

Please request Aurora approval on this when you get a chance.

	Gerry Chang [:gchang]
	Comment 24 • 7 years ago

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

	Brindusa Tot, Desktop QA
	Comment 25 • 7 years ago

I was able to reproduce this bug on Nightly 54.0a1, Build ID 20170227030203 on Windows 10 x64.

Verified as fixed on latest Nightly 55.0a1, Build ID 20170322030208 on Windows 10 x64, Ubuntu 16.04 x64 and Mac 10.10. I verified both options: "Toggle rulers for the page" and "Measure a portion of the page"

	Matteo Ferretti [:zer0] [:matteo] Assignee
	Comment 26 • 7 years ago

Comment on attachment 8848050 [details]
Bug 1342928 - Keep the commands / buttons state in sync;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1320149
[User impact if declined]: Users will have some of the devtools' buttons state out of sync
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: see bug's description
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: DevTools toolbox only
[String changes made/needed]: None

	Gerry Chang [:gchang]
	Comment 27 • 7 years ago

Comment on attachment 8848050 [details]
Bug 1342928 - Keep the commands / buttons state in sync;

Polish the UI issue in devtools and was verified. Aurora54+.

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/9cd943b99c3e