Show error notification when opening RDM on non-remote pages

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Responsive Design Mode
P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

unspecified
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: [multiviewport][reserve-rdm])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

After bug 1240913, we can only open RDM for remote pages.  This fine for viewing web content with e10s on.

However, it means it does not work for pages that load in the parent like about:addons, etc.

Currently we abort and print an error to browser console, but we could improve this by using the browser's notification bar to show the error.

Updated

2 years ago
Flags: qe-verify?

Updated

2 years ago
Priority: P3 → P2
(Assignee)

Updated

2 years ago
Flags: qe-verify? → qe-verify+

Updated

2 years ago
QA Contact: mihai.boldan
(Assignee)

Updated

2 years ago
Blocks: 1250080
(Assignee)

Updated

2 years ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED

Updated

2 years ago
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
Created attachment 8780722 [details]
RDM remote only message

Helen, here's a screenshot of this message we would show when opening RDM on some non-remote tab like about:newtab or about:addons.  What do you think of the text?  It's a bit technical, but would only be shown to DevTools users trying to open the RDM...
Attachment #8780722 - Flags: ui-review?(hholmes)
Comment on attachment 8780722 [details]
RDM remote only message

The screenshot isn't using RDM, but I assume the intended use is that if I had opened new tab in RDM, it would give me this error? Does it appear across the top of the entire browser window, or in the phone viewport?
Flags: needinfo?(jryans)
Attachment #8780722 - Flags: ui-review?(hholmes)
(Assignee)

Comment 4

2 years ago
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #3)
> Comment on attachment 8780722 [details]
> RDM remote only message
> 
> The screenshot isn't using RDM, but I assume the intended use is that if I
> had opened new tab in RDM, it would give me this error? Does it appear
> across the top of the entire browser window, or in the phone viewport?

If you try to open RDM for a non-remote browser tab, such as something from the browser UI like about:newtab (the new tab page) or about:addons (the add-ons manager), we aren't able to open RDM for such a tab.  At the moment if you try to do so, we log an error to the browser console, but there's no indication in the UI of what failed, so it would be fairly confusing, I would imagine.

With the current patch, as shown in the screenshot, you would get an error notification above the browser tab content if this happens.  The error notification is specific to the tab (if you switch to a different tab it is hidden until you return to the problem tab).  The notification can be dismissed by clicking the X on the right hand side.  It's not part of the RDM UI since it can't be shown.

This notification bar is the same UI element used to display various other browser warnings specific to a tab, such as "popup blocked".  You can see the popup warning version by visiting a page such as "http://www.popuptest.com/popuptest5.html".
Flags: needinfo?(jryans) → needinfo?(hholmes)
Ahhh, okay, I understand now. It would appear when you try to open the RDM, I get it now.

I'm happy with the notification and its text. My one question is, should we consider having the notification along the bottom of the page content is a user tries to open RDM with the RDM command-button, as that position would probably be more in their line of view?
Flags: needinfo?(hholmes)
(Assignee)

Comment 6

2 years ago
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #5)
> Ahhh, okay, I understand now. It would appear when you try to open the RDM,
> I get it now.
> 
> I'm happy with the notification and its text. My one question is, should we
> consider having the notification along the bottom of the page content is a
> user tries to open RDM with the RDM command-button, as that position would
> probably be more in their line of view?

I am not sure there's a clearly correct location...  If you use the keyboard shortcut (Cmd-Opt-M) or menu item (Tools -> Web Dev -> RDM) to open RDM, the message could probably appear anywhere as there's not any obviously "related" part of the UI in those cases.  For the command button, I see what you mean about line of view... we do have a similar notification bar inside the toolbox that we could try for the command button case.

The browser UI does already have a very similar notification box that _does_ appear at the bottom of the window, but it's meant for global notifications that impact all tabs in the window, like "this add-on is slow" or "submit this crash report", so I don't think we'd want that specific UI element.

I'll try assembling an new version that uses the toolbox notification bar for the command button case.
(Assignee)

Comment 7

2 years ago
Created attachment 8781301 [details]
RDM remote only message (toolbox button)

Okay, here's how the new version looks when using the toolbox button.  For that case, the message is displayed as part of the toolbox, which places the message near the user's action.

For other cases (shortcut, menu item), we still use the path from the previous screenshot, since there is potentially no toolbox open.

Note that message appearance is a bit different between the two positions... this is because they are implemented entirely differently.  The browser UI notification (top of page) is XUL component that's been around for a while.  The toolbox notification is a new thing for devtools.html, so it seems to differe slightly in appearance.
Attachment #8781301 - Flags: ui-review?(hholmes)
Comment hidden (mozreview-request)

Updated

2 years ago
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29

Comment 9

2 years ago
mozreview-review
Comment on attachment 8780720 [details]
Bug 1274032 - Show notification when opening RDM on non-remote tabs.

https://reviewboard.mozilla.org/r/71366/#review69886

Looks good to me, but there are a few things that I would like to see fixed (or acknowledged at least).
By the way, I noticed that this doesn't work if the user enters a non-remote URL while using the RDM.
The RDM just closes, the non-remote content loads and the tab becomes unusable (you can't set a new URL and lot of things are broken). I'll file a bug if there is not one already.

::: devtools/client/responsive.html/manager.js:40
(Diff revision 2)
>     * @param window
>     *        The main browser chrome window.
>     * @param tab
>     *        The browser tab.
> +   * @param options
> +   *        Other options associated with toggling.  Currently includes:

Nit: there are 2 spaces before "Currently includes" instead of 1.

::: devtools/client/responsive.html/manager.js:62
(Diff revision 2)
>     * @param window
>     *        The main browser chrome window.
>     * @param tab
>     *        The browser tab.
> +   * @param options
> +   *        Other options associated with opening.  Currently includes:

Nit: same problem.

::: devtools/client/responsive.html/manager.js:94
(Diff revision 2)
>     * @param window
>     *        The main browser chrome window.
>     * @param tab
>     *        The browser tab.
> -   * @param object
> -   *        Close options, which currently includes a `reason` string.
> +   * @param options
> +   *        Other options associated with closing.  Currently includes:

Nit: same problem.

::: devtools/client/responsive.html/manager.js:172
(Diff revision 2)
>        case "resize to":
> -        completed = this.openIfNeeded(window, tab);
> +        completed = this.openIfNeeded(window, tab, { command: true });
>          this.activeTabs.get(tab).setViewportSize(args.width, args.height);
>          break;
>        case "resize on":
> -        completed = this.openIfNeeded(window, tab);
> +        completed = this.openIfNeeded(window, tab, { command: true });

Maybe you could factorize a bit here?

```javascript
case "resize to":
    this.activeTabs.get(tab).setViewportSize(args.width, args.height);
case "resize on":
    completed = this.openIfNeeded(window, tab, { command: true });
    break;
```

This may be less readable though, so I'll let you choose.

::: devtools/client/responsive.html/manager.js:225
(Diff revision 2)
> +      if (toolbox) {
> +        nbox = toolbox.notificationBox;
> +      }
> +    }
> +
> +    nbox.appendNotification(

If the user doesn't detect the message instantly and hits the RDM shortcut a few times, this stacks notifications and it's annoying to close them all.
This is not the case for toolbox notifications though.

Can you check here if there's already a notification displayed and bail out early if needed, or remove it and append a new one to draw the user's attention with a new animation?

Comment 10

2 years ago
mozreview-review
Comment on attachment 8780720 [details]
Bug 1274032 - Show notification when opening RDM on non-remote tabs.

https://reviewboard.mozilla.org/r/71368/#review69892
Attachment #8780720 - Flags: review?(bchabod)
Comment on attachment 8781301 [details]
RDM remote only message (toolbox button)

Looks good to me!
Attachment #8781301 - Flags: ui-review?(hholmes) → ui-review+
(Assignee)

Updated

2 years ago
No longer blocks: 1250080
(Assignee)

Updated

2 years ago
See Also: → bug 1250080
Since :bigben's internship has ended, I'll send the next review to :gl.
Comment hidden (mozreview-request)
Comment on attachment 8780720 [details]
Bug 1274032 - Show notification when opening RDM on non-remote tabs.

Let's try :ntim too.  Whoever replies first... :)
Attachment #8780720 - Flags: review?(ntim.bugs)

Comment 15

2 years ago
I couldn't test the patch because it needs rebasing, that aside, the code changes look pretty straightforward.
Flags: needinfo?(jryans)
(Assignee)

Updated

2 years ago
Attachment #8780720 - Attachment is obsolete: true
Attachment #8780720 - Flags: review?(ntim.bugs)
Attachment #8780720 - Flags: review?(gl)
Comment hidden (mozreview-request)
Okay, should be rebased now.
Flags: needinfo?(jryans)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8784058 [details]
Bug 1274032 - Show notification when opening RDM on non-remote tabs.

https://reviewboard.mozilla.org/r/73630/#review71492

Tested locally, works fine!
Attachment #8784058 - Flags: review?(ntim.bugs) → review+

Comment 19

2 years ago
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1732e27db9eb
Show notification when opening RDM on non-remote tabs. r=ntim

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1732e27db9eb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I managed to test this fix on Firefox 51.0a1 (2016-09-01), across platforms[1] and encountered a few potential issues:
- both notification error bars are displayed if trying to open RDM from both Menu bar and Toogle tools. In my opinion, if a notification error is displayed, the second one shouldn't be triggered.
- hovering over the close button 'x' has not the same result on both error bars;
- on Mac and Ubuntu OS's, the the 'x' icon displayed in front of the error message is not the same on both error notification bars;
- the close buttons 'x' are not the same on both error notification bars;
- on Mac OS, the text from notification error bars doesn't have the same format.

Here is a screenshot with the potential issues described above - http://imgur.com/Lfy0Iwo.

[1]Windows 10x64, Mac OS X 10.10.5, Ubuntu 16.04x64
QA Whiteboard: [qe-rdm]
Flags: needinfo?(jryans)
(In reply to Mihai Boldan, QA [:mboldan] from comment #21)
> I managed to test this fix on Firefox 51.0a1 (2016-09-01), across
> platforms[1] and encountered a few potential issues:
> - both notification error bars are displayed if trying to open RDM from both
> Menu bar and Toogle tools. In my opinion, if a notification error is
> displayed, the second one shouldn't be triggered.

Yes, I think we could make a change to improve that.  Feel free to file a new bug to do this.

> - hovering over the close button 'x' has not the same result on both error
> bars;
> - on Mac and Ubuntu OS's, the the 'x' icon displayed in front of the error
> message is not the same on both error notification bars;
> - the close buttons 'x' are not the same on both error notification bars;
> - on Mac OS, the text from notification error bars doesn't have the same
> format.
> 
> Here is a screenshot with the potential issues described above -
> http://imgur.com/Lfy0Iwo.

These are all issues caused by the two different locations using different UI components (they are implemented very differently).  I have copied these issues to bug 1300128 for the group that added the DevTools notification box component to take a look at.

Thanks for testing!
Flags: needinfo?(jryans)
Since the issues described above are covered by Bug 1300128 and Bug 1300489, I am marking this bug Verified Fixed.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
Depends on: 1325557
You need to log in before you can comment on or make changes to this bug.