1325567 - There is no tooltip for the change orientation button

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P3)

Description

[Mihai Boldan, Desktop QA [:mboldan]]

[Affected versions]:
- Firefox 52.0a2 (2016-12-22), Firefox 53.0a1 (2016-12-22)

[Affected platforms]:
- Ubuntu 16.04x64, Windows 8.1x86, Mac 10.12.1

[Steps to reproduce]:
Prerequisites: Make sure that devtools.responsive.html.enabled pref is set to true.

1. Launch Firefox.
2. Enable RDM.
3. Hoover the mouse cursor over the button the rotates the viewport. 

[Expected result]:
- A tooltip is displayed.

[Actual result]:
- Nothing happens.

[Regression range]:
- This is not a regression since the issue is also reproducible on Firefox 47.0a1 (2016-02-11), build where the rotate button was first implemented.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Seems fine to add a tooltip!

To do this, modify the viewport toolbar component in devtools/client/responsive.html/components/viewport-toolbar.js.  The global-toolbar component in the same directory has an example.

See https://wiki.mozilla.org/DevTools/Hacking to get started with the code base.

Comment 2

[Deepjyoti Mondal]

Attachment: bug1325567.patch

Added tooltip to the rotate viewport button

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8822933 [details] [diff] [review]
bug1325567.patch

Review of attachment 8822933 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this!

Please add "r=jryans" to the end of the commit message.

Looks like one small tweak to make.

::: devtools/client/locales/en-US/responsive.properties
@@ +18,5 @@
>  # LOCALIZATION NOTE (responsive.exit): tooltip text of the exit button.
>  responsive.exit=Close Responsive Design Mode
>  
> +# LOCALIZATION NOTE (responsive.rotate): tooltip text of the rotate button.
> +responsive.rotate=Rotate Viewport

I believe "Rotate viewport" is slightly better, we typically use "sentence" casing for tooltips.  (The one above this is not a very good example, since "Responsive Design Mode" is a proper noun, and so it's capitalized.

Comment 4

[Deepjyoti Mondal]

Attachment: bug1325567.patch

Updated my previous patch.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8823801 [details] [diff] [review]
bug1325567.patch

Review of attachment 8823801 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this looks good to me!  Thanks for working on it!

Here's a try run to make sure tests agree:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c754b210f1924f68e33802775de1a49878c67dc8

Once it looks good, we can add the "checkin-needed" keyword to this bug for landing.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Please take a look at http://firefox-dev.tools for more ideas of bugs to work on.  Thanks again!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc2ec20e998
Added tooltip to rotate viewport button. r=jryans

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8dc2ec20e998

Comment 9

[Francesco Lodolo [:flod]]

Suggestion for the future: in lack of a localization note, using "tooltip" in the string ID helps understanding what's happening. Tooltips, unlike labels, don't have much of a length constraint, and might require a different style (description) than a button label (action).

Comment 10

[Mihai Boldan, Desktop QA [:mboldan]]

The bug is no longer reproducible on Firefox 53.0a1 (2017-01-09).
The tests were performed under Mac OS X 10.11, Windows 10x64 and under Ubuntu 16.04x64.