Closed Bug 1340553 Opened 3 years ago Closed 3 years ago

The rulers are not dismiss if using the 'rulers' command in Developer Toolbar

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 verified)

VERIFIED FIXED
Firefox 54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- verified

People

(Reporter: mboldan, Assigned: zer0)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

[Note]:
- Logged based on Bug 1144163 Comment 38

[Affected versions]:
- Firefox 51.0.1, Firefox 52.0b6, Firefox 53.0a2 (2017-02-17), Firefox 54.0a1 (2017-02-17)

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

[Steps to reproduce]:
1. Launch Firefox.
2. Enable Toogle tools (Ctrl+Shift+I).
3. Enable Developer Toolbar (Shift+F2).
4. Open Toolbox Options and enable 'Toogle rulers for the page' option.
5. Enable the Rulers by clicking on the rulers icon.
6. Disable the rulers by using the 'rulers' command in the Developer Toolbar.

[Expected result]:
- Rulers tool is disabled.

[Actual result]:
- Nothing happens.

[Regression range]:
- This is not a regression.
Here is the result from mozregression.
Last good revision: 0496b5b3e9ef (2015-06-05)
First bad revision: 4a07e1ac3cdf (2015-06-06)

[Additional notes]:
- Note that if Toggle tools is dismissed and the rulers command is used in Developer Toolbar, the Rulers icon from Toggle tools is disabled and the rulers tool is still displayed on the screen.
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Assignee: nobody → zer0
Joe, here the patches as we discussed already. I think the only difference is that I didn't add any resolved promises by default for `hidePromise` or `showPromise`. I decided to do that since Task.async handle `null` values (as the current developer-toolbar code does), and in this case we can have basically three state: 

1. `show/hidePromise` is `null`, therefore `show/hide` was never called, or the object is destroyed
2. `show/hidePromise` is resolved, therefore the toolbox's show/hide method has finished
3. `show/hidePromise` is not `null`, it means it can be either in the process of showing/hiding or resolved. If we keep the `null` state, we don't care about the difference, but we need a way to understand if, at least, the method was called.

Let me know if this is fine to you. A preliminary try build on linux didn't show any related issues, and manual tests seems shown that the current bug is fixed.
Comment on attachment 8843136 [details]
Bug 1340553 - The rulers are not dismiss if using the 'rulers' command in Developer Toolbar;

https://reviewboard.mozilla.org/r/116910/#review118632

We're making the hidden/visible state even more complex with these changes. Could you add some documentation to help a future editor of this file understand how to know whether the toolbar is open/opening/closed/closing.

::: devtools/client/shared/developer-toolbar.js:482
(Diff revision 2)
>    if (this._hidePromise != null) {
>      return this._hidePromise;
>    }
>  
>    // show() is async, so ensure we don't need to wait for show() to finish
> -  let waitPromise = this._showPromise || promise.resolve();
> +  this._hidePromise = this._showPromise.then(() => {

Now if you call hide() before calling show() it will crash, right? This wasn't the case before.
Comment on attachment 8843136 [details]
Bug 1340553 - The rulers are not dismiss if using the 'rulers' command in Developer Toolbar;

https://reviewboard.mozilla.org/r/116910/#review118632

I tried to be a more clear about it, and I noticed also that neither `_showPromise` nor `_hidePromise` were declared, so I fixed that too. Let me know if it's fine now or if you have any suggestion about that.

> Now if you call hide() before calling show() it will crash, right? This wasn't the case before.

You're totally right, thanks for the catch! I added the check for this scenario, so it would returns a resolved promise in this case.
I address your comments, it's ready for a second round! Thanks Joe.
Flags: needinfo?(jwalker)
Comment on attachment 8843136 [details]
Bug 1340553 - The rulers are not dismiss if using the 'rulers' command in Developer Toolbar;

https://reviewboard.mozilla.org/r/116910/#review118930

Thanks
Attachment #8843136 - Flags: review?(jwalker) → review+
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84c1dd84d182
The rulers are not dismiss if using the 'rulers' command in Developer Toolbar; r=jwalker
https://hg.mozilla.org/mozilla-central/rev/84c1dd84d182
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Is this worth uplifting to soon-to-be-beta 53?
I managed to reproduce this issue on Firefox 53.0a2 (2017-03-05), under Windows 10x64.
I also performed a set of tests on Firefox 55.0a1 (2017-03-06), under Mac OS X 10.11.6, Windows 10x64 and on Firefox 54.0a1 (2017-03-06) and noticed that the tool is dismissed after step 6 form STR from Comment 0, but the button remains enabled (not sure if covered by Bug 1342928). 
Also if clicking on the button from the Toolbox, after Step 6, the tool is enabled, but the button is disabled. 

Matteo, what's your opinion about this?
(In reply to Mihai Boldan, QA [:mboldan] from comment #13)
> I managed to reproduce this issue on Firefox 53.0a2 (2017-03-05), under
> Windows 10x64.
> I also performed a set of tests on Firefox 55.0a1 (2017-03-06), under Mac OS
> X 10.11.6, Windows 10x64 and on Firefox 54.0a1 (2017-03-06) and noticed that
> the tool is dismissed after step 6 form STR from Comment 0

So the misbehavior is gone after this patch, that's correct?

> but the button
> remains enabled (not sure if covered by Bug 1342928).
> Also if clicking on the button from the Toolbox, after Step 6, the tool is
> enabled, but the button is disabled.

> Matteo, what's your opinion about this?

It seems that since bug 1320149, the toolbox and the toolbar doesn't work well together, basically the toolbar's command doesn't trigger a change of state in the toolbox's buttons. But it's not part of this bug, I will check how to fix that in bug 1342928.
(In reply to Julien Cristau [:jcristau] from comment #12)
> Is this worth uplifting to soon-to-be-beta 53?

Julien, I don't think is worth an uplifiting.
The developer toolbar added multiple event listeners since a while and we didn't notice that until now. You can notice only if the user click multiple times on the toolbox's settings when the toolbar is open; and use a tool that has a toggle state, and such tool is both in toolbox and toolbar, and he switching from clicking the icon in the toolbar and typing the same command in the toolbox. Ah, and he has to click a even number of times in the toolbox's settings (since is a toggle command, odd number of times will basically give you the same result of having one listener).
Flags: needinfo?(zer0)
Flags: needinfo?(jwalker)
I have reproduced this bug with Nightly 54.0a1 (2017-02-17) on Windows 10, 64 bit!

The bug's fix is now verified on Latest Aurora 54.0a2

Build ID 	20170412004024
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170412]
Flags: qe-verify+
Thanks Tanvir for testing this issue.

The issue is no longer reproducible on Firefox 54.0b2.
Tests were performed under Mac OS X 10.12.1, Windows 10x64, Ubuntu 16.04x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.