Closed Bug 1428816 Opened 6 years ago Closed 6 years ago

Offer user control over RDM refresh behavior

Categories

(DevTools :: Responsive Design Mode, enhancement, P2)

Product:
DevTools
Component:
Responsive Design Mode
Type:
enhancement

Tracking

(relnote-firefox 60+, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
relnote-firefox --- 60+
firefox60 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

Bug 1428816 - Rename deviceListState to loadableState.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1428816 - Add RDM UI to control whether we reload.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1428816 - Show reload help on first RDM open.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
vertical text align
8.29 KB, image/png
Details 
Currently, RDM will auto-refresh the page when toggling touch simulation state.  

This is done so that touch APIs (which most pages feature detect at load time) are actually used by the page under test.

However, they are many workflows where you don't care about touch in RDM.  If you are just checking the layout at different sizes, these refreshes waste time and even cause loss of work if you have made temporary changes in the DevTools that you haven't persisted (bug 1401196).

Since it is impossible for the tool to know what type of workflow the user is attempting, we should offer user control over whether refreshes are done or not.

Possible UI idea
----------------

New button in the global toolbar (top toolbar) that says "Reload when..." (or has an appropriate icon, with that as tooltip).  This opens a dropdown when clicked:

Reload when...
|_ touch simulation is toggled
|_ user agent is changed

We would change the default behavior to _never_ reload.  Instead, the first time that one of touch or UA is changed, we'll show a notification that reload is needed to fully apply, and can be automated by enabling in the menu.
(Not sure if this is the right process to request UX feedback on a random bug unrelated to an active project.  Please let me know if there's a better way!)

Victoria, what do you think about the problem above and the UI idea I mentioned?  Please let me know if more context is needed.

I just noticed my text mockup above doesn't show the checked state...  Here's a more accurate version:

Reload when...
|_ [ ] touch simulation is toggled
|_ [ ] user agent is changed

When you select an item in the menu, it toggles the checkbox for that reload condition.
Flags: needinfo?(victoria)
(For UX request time sensitivity, we have feedback that the current situation is frustrating for users, so I would hope to make a change soonish, but we've also been living with the current state for a while.  So maybe that's "medium" urgency?)
Thanks J. Ryan. This does seem like an important problem, especially for user agent changes - my workflow would be the same as the one reported in 1401196.

I like your solution - there's plenty of space in that top bar so it seems good to use "Reload when..." for the default dropdown item. (I think there might be a more OS-consistent way to word this - will let you know if I think of something better.) 

I would show the warning for both first uses of touch and UA. Maybe a "Don't show again" checkbox instead of just first launch behavior would be safer. It's such a tricky situation when both results can be kind of disastrous - being inaccurate or destroying work.

I seem to remember that Chrome used to show a yellow warning banner in the page when switching user agents, which would invite the user to refresh. They don't seem to do this anymore - the layout could be incorrect after switching agents but a manual refresh would always be needed. That they've gone this far in the direction of never refreshing is surprising and raises the priority of fixing this for me.

For a simple way forward, we could reuse the warning banner currently used for the Debugger paused state, which shows up just above the DevTools. In the future, I would love to work on the RDM toolbar more and include a banner option that can show up close to the top.
Flags: needinfo?(victoria)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
As a side note, bug 1381463 added an extra reload transition (when leaving RDM) in 59.  For users who rely on accuracy of touch APIs, the reloads are important, but for those who do not, this potentially adds more frustration.
See Also: → 1381463
Release Note Request (optional, but appreciated)
[Why is this notable]: Highly requested feature by developers
[Affects Firefox for Android]: No
[Suggested wording]: Responsive Design Mode now offers explicit control over whether the page should reload
[Links (documentation, blog post, etc)]: MDN docs to be written
relnote-firefox: --- → ?
(In reply to Victoria Wang [:victoria] from comment #3)
> For a simple way forward, we could reuse the warning banner currently used
> for the Debugger paused state, which shows up just above the DevTools. In
> the future, I would love to work on the RDM toolbar more and include a
> banner option that can show up close to the top.

For the banner, I don't think I can easily reuse this debugger one, as it's part of the DevTools toolbox, and you can have RDM open without the toolbox displayed.

For now, I'll try using the browser-level banner UI that appears at the top of the tab, and we'll see how we like that.
Victoria: here's a macOS try build for this feature.  Please let me know what you think! :)

https://queue.taskcluster.net/v1/task/SVUYX4K9Q96USIFsYEDmCw/runs/0/artifacts/public/build/target.dmg
Flags: needinfo?(victoria)
This looks great! Love the use of the browser-level banner. I wonder if it should persist until clicking the x button.

For the wording, might be clearer to say "Automatic reloads are disabled..."

It would be helpful to have one of those double-arrow icons next to the reload dropdown.

Some bonus ideas that could be dealt with in a separate patch: 

Adding some additional splitter lines would really help the look of the RDM toolbar. Maybe something like this on all three select elements in the top bar would be nice:

background-position-x: right 5px;
border-right: 1px solid var(--theme-splitter-color);
padding-right: 20px;

Any non-deactivated dropdown could be darker looking, like --theme-body-color.

Also, I think it will look clearer if the two buttons for touch and screenshot are moved back to being next to the x button.
Flags: needinfo?(victoria)
Comment on attachment 8949600 [details]
Bug 1428816 - Rename deviceListState to loadableState.

https://reviewboard.mozilla.org/r/218968/#review225372
Attachment #8949600 - Flags: review?(poirot.alex) → review+
Attached image vertical text align — 
I don't know how it looks on other OSes, but on linux, the checkboxes aren't aligned correctly next to their labels.
Comment on attachment 8949601 [details]
Bug 1428816 - Add RDM UI to control whether we reload.

https://reviewboard.mozilla.org/r/218970/#review225376

It all looks good to me, but I'm not sure I'm the best reviewer for React/Redux patches like this one.

Otherwise the patches work great. Sometime I clicked on the label and it did not toggled the checkboxes. I think it is related to misalignement of labels.

Otherwise regarding UX, I'm wondering if such menu should be so visible compared to all the others.
Right now it is as big as "throttling", whereas it sounds less important.
I was wondering if it could be helpful to introduce a gearing icon, like about:newtab with random less important settings. Then, the browser notification would refer to that.
Attachment #8949601 - Flags: review?(poirot.alex) → review+
Comment on attachment 8949602 [details]
Bug 1428816 - Show reload help on first RDM open.

https://reviewboard.mozilla.org/r/218972/#review225380

Looks good as well.

Again, only speaking about UX, I was wondering if it would help to have an inlined notification, within RDM document, like a doorhanger popup, which would help saying where the menu is instead of writing down in the notification where to look for it.
Attachment #8949602 - Flags: review?(poirot.alex) → review+
Ah, thanks Alex! I like your idea about moving to a gear menu, assuming that people won't change this often (e.g. it's a per-project setting rather than per-session). My only concern is to not delay the main part of this patch — the not-auto-reloading part, since I see it as kind of urgent. Maybe the gear could be part of a separate polish patch? Or maybe (since this is slated for 60, I believe) we have plenty of time and I'm worrying too much. 

Re: notification: I like the top banner treatment since it's serving as an announcement of changed behavior affecting the whole RDM; it's not just a notice about the dropdown itself.
(In reply to Victoria Wang [:victoria] from comment #12)
> This looks great! Love the use of the browser-level banner. I wonder if it
> should persist until clicking the x button.

Do you think the banner should appear immediately on RDM open?  Or should we wait until one of the emulation features (touch, user agent) is first activated (mostly likely by selecting a device)?

> For the wording, might be clearer to say "Automatic reloads are disabled..."

Ah thanks, I agree!

> It would be helpful to have one of those double-arrow icons next to the
> reload dropdown.

Ah, the ones next to the existing select dropdowns?

> Some bonus ideas that could be dealt with in a separate patch: 
> 
> Adding some additional splitter lines would really help the look of the RDM
> toolbar. Maybe something like this on all three select elements in the top
> bar would be nice:
> 
> background-position-x: right 5px;
> border-right: 1px solid var(--theme-splitter-color);
> padding-right: 20px;
> 
> Any non-deactivated dropdown could be darker looking, like
> --theme-body-color.

Good ideas, I'll give these a try!

> Also, I think it will look clearer if the two buttons for touch and
> screenshot are moved back to being next to the x button.

Okay, I'll put the reload menu before these buttons, so they are back together with Exit again.
Flags: needinfo?(victoria)
(In reply to Victoria Wang [:victoria] from comment #17)
> Ah, thanks Alex! I like your idea about moving to a gear menu, assuming that
> people won't change this often (e.g. it's a per-project setting rather than
> per-session). My only concern is to not delay the main part of this patch —
> the not-auto-reloading part, since I see it as kind of urgent. Maybe the
> gear could be part of a separate polish patch? Or maybe (since this is
> slated for 60, I believe) we have plenty of time and I'm worrying too much. 

For now, I'd like to offer something very "direct" about refreshing, so that it's clear to users we're addressing it.  I wouldn't want it to get lost in a mysterious gear menu.  (I agree the refresh menu does take up a lot of toolbar room relative to its importance, though.)

I'd say let's go with what we have here for now, and then consider additional polish separately.
(In reply to Alexandre Poirot [:ochameau] from comment #14)
> Created attachment 8950362 [details]
> vertical text align
> 
> I don't know how it looks on other OSes, but on linux, the checkboxes aren't
> aligned correctly next to their labels.

Ah thanks, I'll fix this up.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> Do you think the banner should appear immediately on RDM open?  Or should we
> wait until one of the emulation features (touch, user agent) is first
> activated (mostly likely by selecting a device)?

Ah, I'm not sure - on one hand, having it show up immediately works well with my "announcement" idea (especially if we add the word "now" in there somewhere). I'd be fine with keeping it as is for now since it already works this way.

However, if the banner was associated with an emulation change, we could show a more specific and helpful message. I fear that most people don't know what "touch and user agent simulation" means, but they know they can "change it to phone" or whatever. If, upon changing user agent, the message said "Note: Device simulation changes require a reload to fully apply. Automatic reloads are disabled..." etc, that would be a lot more educational.

> > It would be helpful to have one of those double-arrow icons next to the
> > reload dropdown.
> 
> Ah, the ones next to the existing select dropdowns?

Yep!
 
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19)
> For now, I'd like to offer something very "direct" about refreshing, so that
> it's clear to users we're addressing it.  I wouldn't want it to get lost in
> a mysterious gear menu.  (I agree the refresh menu does take up a lot of
> toolbar room relative to its importance, though.)
> 
> I'd say let's go with what we have here for now, and then consider
> additional polish separately.

Sounds good!
Flags: needinfo?(victoria)
(In reply to Victoria Wang [:victoria] from comment #12)
> Any non-deactivated dropdown could be darker looking, like
> --theme-body-color.

About the text color, what states exactly do you mean by "non-deactivated"?  The currently styling on the toolbar dropdowns is a bit unique:

* default: --viewport-color = #999797 (light), #c6ccd0 (dark)
* hover: --viewport-hover-color = #4a4a4f (light), #dde1e4 (dark)
* dropdown feature active: --viewport-active-color = #3b3b3b (light), #fcfcfc (dark)

(The icon-style buttons are different when active, they use --theme-toolbar-checked-color like other parts of DevTools.)

A lot of these color are quite similar, so these states are quite hard to tell apart. This all probably needs rethinking when get around to refreshing the RDM look at some stage.

So, is there a specific state here that you'd like to change out of these?  Do you mean the default color?  (Using --theme-body-color would make the light theme's default and hover colors the same.)
Flags: needinfo?(victoria)
(In reply to Victoria Wang [:victoria] from comment #21)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> > Do you think the banner should appear immediately on RDM open?  Or should we
> > wait until one of the emulation features (touch, user agent) is first
> > activated (mostly likely by selecting a device)?
> 
> Ah, I'm not sure - on one hand, having it show up immediately works well
> with my "announcement" idea (especially if we add the word "now" in there
> somewhere). I'd be fine with keeping it as is for now since it already works
> this way.
> 
> However, if the banner was associated with an emulation change, we could
> show a more specific and helpful message. I fear that most people don't know
> what "touch and user agent simulation" means, but they know they can "change
> it to phone" or whatever. If, upon changing user agent, the message said
> "Note: Device simulation changes require a reload to fully apply. Automatic
> reloads are disabled..." etc, that would be a lot more educational. 

I tried it the other way (only appears on emulation change), and I think it feels better.  This way, the UI is a bit cleaner the first time you open it (you won't get bombarded by the notification right away, which might be overwhelming if you've never used the tool before).

I'll post a new build in case you want to try out this version.
Thanks for the new build - this is looking great!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22)
> (In reply to Victoria Wang [:victoria] from comment #12)
> > Any non-deactivated dropdown could be darker looking, like
> > --theme-body-color.
> 
> About the text color, what states exactly do you mean by "non-deactivated"? 
> The currently styling on the toolbar dropdowns is a bit unique:
> 
> * default: --viewport-color = #999797 (light), #c6ccd0 (dark)
> * hover: --viewport-hover-color = #4a4a4f (light), #dde1e4 (dark)
> * dropdown feature active: --viewport-active-color = #3b3b3b (light),
> #fcfcfc (dark)
> 
> (The icon-style buttons are different when active, they use
> --theme-toolbar-checked-color like other parts of DevTools.)
> 
> A lot of these color are quite similar, so these states are quite hard to
> tell apart. This all probably needs rethinking when get around to refreshing
> the RDM look at some stage.
> 
> So, is there a specific state here that you'd like to change out of these? 
> Do you mean the default color?  (Using --theme-body-color would make the
> light theme's default and hover colors the same.)

Thanks for the info! By "non-deactivated" I meant the default color for any dropdown that was accessible (I think the DPR one was invalid for me on some site?). I like the default color being darker for contrast and to show that it's active - I'm not sure if dropdowns need a hover state (MacOS doesn't do this so it feels non-native to me). This is all super nitpicky though - I do agree that it makes sense to polish this later since there are a lot more changes I'd want to make when we get to that. :)
Flags: needinfo?(victoria)
(In reply to Victoria Wang [:victoria] from comment #28)
> Thanks for the info! By "non-deactivated" I meant the default color for any
> dropdown that was accessible (I think the DPR one was invalid for me on some
> site?). I like the default color being darker for contrast and to show that
> it's active - I'm not sure if dropdowns need a hover state (MacOS doesn't do
> this so it feels non-native to me). This is all super nitpicky though - I do
> agree that it makes sense to polish this later since there are a lot more
> changes I'd want to make when we get to that. :)

Okay, sounds like we can leave these colors as they are for now and return to polish in detail when we're focused on that.  (I do agree it feels a bit strange as it is today!)
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0d00eabc8b83
Rename deviceListState to loadableState. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a6e7a7b21cf3
Add RDM UI to control whether we reload. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/31e12673218b
Show reload help on first RDM open. r=ochameau
Backed out 3 changesets (bug 1428816) for Browser Chrome failures on browser/base/content/test/static/browser_misused_characters_in_strings.js on a CLOSED TREE
Link to the log https://treeherder.mozilla.org/logviewer.html#?job_id=163720010&repo=autoland&l
Backout revision https://hg.mozilla.org/integration/autoland/rev/2e44a7285ccddce48e794db74ce59cd98fd1ae9f
Failed push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=31e12673218bf3b8fb88f32b38c361ce65a2ca18
Flags: needinfo?(jryans)
Flags: needinfo?(jryans)
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8cd33fe4b971
Rename deviceListState to loadableState. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/dc1bd6cc0ade
Add RDM UI to control whether we reload. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c8d05dc037db
Show reload help on first RDM open. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/8cd33fe4b971
https://hg.mozilla.org/mozilla-central/rev/dc1bd6cc0ade
https://hg.mozilla.org/mozilla-central/rev/c8d05dc037db
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment on attachment 8949602 [details]
Bug 1428816 - Show reload help on first RDM open.

https://reviewboard.mozilla.org/r/218972/#review228552

::: devtools/client/locales/en-US/responsive.properties:161
(Diff revision 3)
>  responsive.reloadConditions.userAgent=Reload when user agent is changed
> +
> +# LOCALIZATION NOTE (responsive.reloadNotification.description): Text in notification bar
> +# shown on first open to clarify that some features need a reload to apply.  %1$S is the
> +# label on the reload conditions menu (responsive.reloadConditions.label).
> +responsive.reloadNotification.description=Device simulation changes require a reload to fully apply.  Automatic reloads are disabled by default to avoid losing any changes in DevTools.  You can enable reloading via the “%1$S” menu.

Note for the future: strings in Firefox don't use double space after a punctuation. Also, usually not much of an effect on HTML rendering.
(In reply to Francesco Lodolo [:flod] (offsite until Feb 25, slow replies) from comment #37)
> Note for the future: strings in Firefox don't use double space after a
> punctuation. Also, usually not much of an effect on HTML rendering.

Noted, I'll attempt to remember... but as you say, it doesn't change the rendered output vs. single space.
added to draft 60.0beta release notes
relnote-firefox: ? → 60+
I've added a section covering this new feature, and a note about it in the Fx 60 rel notes:

https://developer.mozilla.org/en-US/docs/Tools/Responsive_Design_Mode#Controlling_page_reload_behavior
https://developer.mozilla.org/en-US/Firefox/Releases/60#Developer_tools

Let me know if this looks OK. Thanks!
Flags: needinfo?(jryans)
Keywords: dev-doc-needed → dev-doc-complete
Thanks Chris, this looks great!
Flags: needinfo?(jryans)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: