970047 - Add a 2nd screen button for full page screenshot (responsive mode)

Status: RESOLVED WONTFIX

(DevTools :: Responsive Design Mode, enhancement)

Description

[Florens Verschelde :fvsch]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140208040203

Steps to reproduce:

Feature request: add a full-page screenshot button to the Responsive Design View (aka Responsive Mode). Currently the screenshot button only allows users to take a screenshot of the visible content.

Rationale: full-page screenshots are a common need for web designers and developers, as exemplified by popular screenshot extensions for Firefox, Chrome, etc. This feature is generally the core reason for those extensions to exist, since operating systems provide their own screenshot feature to capture visible content.

This feature is already available by combining the Responsive Design View (to get the viewport width you need for testing/demonstration) and the Developer Toolbar command line. I’d argue that a majority of users of the Responsive Design View are unlikely to ever use the nerdy Developer Toolbar, especially web designers, so exposing the "full page" screenshot feature in the Responsive Design View UI would make sense.

The bug for adding the current screenshot button was: #849236

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

A screenshot button would make sense but for now you can use the screenshot command in the Firefox Command Line (SHIFT + F2).

Comment 2

[Paul Rouget [:paul]]

I think that makes sense. Should we add a new button? Take a fullpage screenshot by default? Or maybe the saved screenshot could include both images, like that: http://i.imgur.com/DaTdzUv.png

Comment 3

[Florens Verschelde :fvsch]

I suspect having both screenshots in the same image would bother users who just want one, and those who want to reuse a screenshot in a presentation, a blog post, or anywhere else. It also would give “bad” results for long pages: when opening or previewing the image, the viewport screenshot would appear tiny.

I suggest one of these two solutions:
- Two buttons
- One drop-down with two options

I’ll add a mock-up for the drop-down option (not sure this style can be achieved but still adding it for reference).

Comment 4

[Paul Rouget [:paul]]

(In reply to Florent Verschelde from comment #3)
> I suggest one of these two solutions:
> - Two buttons
> - One drop-down with two options
> 
> I’ll add a mock-up for the drop-down option (not sure this style can be
> achieved but still adding it for reference).

I think we can just add a 2nd button. These buttons are narrow. So I guess it would work.

Comment 5

[Florens Verschelde :fvsch]

Attachment: Mock-up: button with 2-choice dropdown

The interaction model for this button would be the same as chrome nav bar/tab bar buttons which trigger a drop-down menu. No idea if it’s doable in this context.

Comment 6

[Paul Rouget [:paul]]

Comment on attachment 8373993 [details]
Mock-up: button with 2-choice dropdown

I like that.

Comment 7

[Paul Rouget [:paul]]

(In reply to Florent Verschelde from comment #5)
> Created attachment 8373993 [details]
> Mock-up: button with 2-choice dropdown
> 
> The interaction model for this button would be the same as chrome nav
> bar/tab bar buttons which trigger a drop-down menu. No idea if it’s doable
> in this context.

Yep. Doable.

Comment 8

[Lancelot Nyachoto]

Hi. I would like to take up this task with some guidance as this will be my very first bug.

Comment 9

[Paul Rouget [:paul]]

(In reply to Lancelot Nyachoto from comment #8)
> Hi. I would like to take up this task with some guidance as this will be my
> very first bug.

Sure. Code is located here: /browser/devtools/responsivedesign/responsivedesign.jsm

(http://mxr.mozilla.org/mozilla-central/source/browser/devtools/responsivedesign/responsivedesign.jsm)

To implement something like attachment 8373993 [details], you'll need to use a toolbarbutton with a menupoup, like in the webconsole (see http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.xul#145).

Comment 10

[Lancelot Nyachoto]

Thanks Paul. I have downloaded mozilla-central and have a working build. I will start hacking responsivedesign.jsm and come back you if I have any questions.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Lancelot Nyachoto from comment #10)
> Thanks Paul. I have downloaded mozilla-central and have a working build. I
> will start hacking responsivedesign.jsm and come back you if I have any
> questions.

Oops, you should be using the fx-team repo. For mercurial that would be:
hg clone http://hg.mozilla.org/integration/fx-team

Paul was just pointing out which files you should be targetting.

Comment 12

[Lancelot Nyachoto]

Attachment: Screenshot of in progress popup menu

Comment 13

[Lancelot Nyachoto]

Thanks Michael, I will clone the correct repository.  I guess this will result in a better debugging/development workflow, which I'm still trying to figure out? Is there a page with instructions on how to do this efficiently? Currently I'm editing the code and running 
./mach build 
and thereafter 
./mach run -p developer 
which takes a while to complete one cycle.

I had managed to get a drop down menu as shown in the attachment https://bugzilla.mozilla.org/attachment.cgi?id=8375784 by updating the function RUI_buildUI() in the file /browser/devtools/responsivedesign/responsivedesign.jsm.  I still need to figure out how XUL will come into the equation, some guidance here will be appreciated.

Where does this function "this.strings.GetStringFromName()" get the string data from?

Comment 14

[Paul Rouget [:paul]]

Looks great!

(Make sure that we see the menu only if we click on the arrow)

> ./mach build browser/devtools browser/locales browser/theme
> ./mach run -P developer

This shoud work for you.

You'll find the strings here: /browser/locales/en-US/chrome/browser/devtools/responsiveUI.properties

Comment 15

[Lancelot Nyachoto]

Attachment: Test: Visible Area Image output

Comment 16

[Lancelot Nyachoto]

Attachment: Test: Full Page Area Image output

Comment 17

[Lancelot Nyachoto]

Thanks Paul, this greatly improved my workflow :)

Just to keep you posted, I have modified the function RUI_screenshot to take a new parameter aMode which will determine whether to take a full page or visible area screenshot and it is working as per the following screenshots.

Visible area: https://bugzilla.mozilla.org/attachment.cgi?id=8376209
Full page area: https://bugzilla.mozilla.org/attachment.cgi?id=8376210

I'm now working on the screenshot button, to make sure we only see the menu if we click on the arrow.

Comment 18

[Lancelot Nyachoto]

Oh, by the way, how do I get assigned to this bug on bugzilla as it is not appearing on my dashboard?

Comment 19

[Paul Rouget [:paul]]

(In reply to Lancelot Nyachoto from comment #17)
> Thanks Paul, this greatly improved my workflow :)
> 
> Just to keep you posted, I have modified the function RUI_screenshot to take
> a new parameter aMode which will determine whether to take a full page or
> visible area screenshot and it is working as per the following screenshots.
> 
> Visible area: https://bugzilla.mozilla.org/attachment.cgi?id=8376209
> Full page area: https://bugzilla.mozilla.org/attachment.cgi?id=8376210
> 
> I'm now working on the screenshot button, to make sure we only see the menu
> if we click on the arrow.

Excellent! Thank you.

Comment 20

[Lancelot Nyachoto]

Attachment: Final Screenshot Popup Menu

I hope this is satisfactory?

Comment 21

[Paul Rouget [:paul]]

(In reply to Lancelot Nyachoto from comment #20)
> Created attachment 8376287 [details]
> Final Screenshot Popup Menu
> 
> I hope this is satisfactory?

Looks good. Not sure why the arrow is different from the menu on the left.
toolbarbuttons with menupopups are hard to style and get right.

Attach a patch, mark it as `f? :paul` and I'll take a look.

Comment 22

[Lancelot Nyachoto]

> 
> Attach a patch, mark it as `f? :paul` and I'll take a look.

How do I go about doing this? Do I just use hg diff and attach the file like I did with the images?

Comment 23

[Paul Rouget [:paul]]

(In reply to Lancelot Nyachoto from comment #22)
> > 
> > Attach a patch, mark it as `f? :paul` and I'll take a look.
> 
> How do I go about doing this? Do I just use hg diff and attach the file like
> I did with the images?

Yep, that should work. You might want to read this: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 24

[Lancelot Nyachoto]

Attachment: Bug-970047.patch

Patch for review

Comment 25

[Paul Rouget [:paul]]

Comment on attachment 8376683 [details] [diff] [review]
Bug-970047.patch

You want to use a menubutton:

> <toolbarbutton type="menu-button">

which would come with a menupopup. With your approach, only the menuitems are clickable. The button needs to be blockable, and the items should be checkboxes. Then the user would need to click on the arrow only when he wants to change the screenshot behavior.

Documentation here: https://developer.mozilla.org/en-US/docs/XUL/toolbarbutton

An example: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.xul#145

Comment 26

[Lancelot Nyachoto]

Oh, I see what you mean. I will work on that and get back to you.

Comment 27

[Paul Rouget [:paul]]

s/blockable/clickable/

Comment 28

[Lancelot Nyachoto]

Sorry for taking a while to respond, I have been tied up at work.

I have managed to make the button clickable.

Question: Can the user select both options (visible & full page) at once or is only allowed to select one option at a time?

Comment 29

[Lancelot Nyachoto]

Attachment: Bug-970047-12032014.patch

Made the touchscreen blockable and the menu items check boxes.
If the user selects both options "Visible Area" and "Fullpage" both screenshot version are generated
If none are selected the Screenshot button is disabled (might need to discuss if this results in negative user experince)

Comment 30

[Lancelot Nyachoto]

Attachment: Bug-970047.patch

Comment 31

[Lancelot Nyachoto]

Hi Paul. Just checking if this bug is still valid?

Comment 32

[Paul Rouget [:paul]]

Sorry, yes, it's still valid. I'm super busy right now, I think I'll have time to look at this in a couple of weeks.

Comment 33

[Mike Hoye [:mhoye]]

Hi, Lancelot - Are you still working on this?

Comment 34

[Mike Hoye [:mhoye]]

Ah, my mistake - Paul, will you have a chance to look at this soon?

Comment 35

[Paul Rouget [:paul]]

Comment on attachment 8399876 [details] [diff] [review]
Bug-970047.patch

Sorry for the very very late review :(

It looks good, but I'd prefer if there would be only one possible toggle.

Either use 2 radio buttons:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/radio

> ( ) visible area
> (*) full page

Or only one checkbox:

> [x] only visible area

Use only one pref: devtools.responsiveUI.fullpage (false by default), which would be toggled when the checkbox or the radio buttons change.

The user should not be able to disable both options or select both options.

Comment 36

[Lancelot Nyachoto]

Attachment: Bug970047-18-06-2014.patch

Added one possible toggle:  [x] visible area only

However if the responsive view is opened with the menu item "visible area only
" unchecked, the check button is not being toggled (to visually show as checked or unchecked), though the value of the pref: devtools.responsiveUI.fullpage is being set correctly. I need some assistance on that.

Comment 37

[Anthony Ricaud (:rik)]

Is this still needed now that bug 991045 landed?

Comment 38

[Paul Rouget [:paul]]

(In reply to Anthony Ricaud (:rik) from comment #37)
> Is this still needed now that bug 991045 landed?

Are you suggesting we should remove the screenshot button from the responsive view since we have a screenshot button in the toolbox?

I'll let Patrick decide.

Comment 39

[Patrick Brosset <:pbro>]

Well, the new screenshot toolbar button is optional and hidden by default, and can only take full-pages screenshots. So assuming there's an audience already used to the responsive mode that didn't see the new button land, I'd keep the extra button in the responsive view. Just because it's probably useful to have it there and doesn't use any super import UI real estate.
But that's just my opinion.

Comment 40

[Lancelot Nyachoto]

So what's the resolution?

Comment 41

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

No updates in many months, unassigning.

Comment 42

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 43

[Pavan Karthik [:matrixisreal]]

Hi,this looks like a very old bug, is it still active. If it is, i would love to work on this,itll be my first bug fix.
I have few doubts before getting started,
I've cloned the mozilla-central repo on my linux machine , is that ok for working on this bug.
I think this is it so far :D
Thanks.

Comment 44

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

Thanks Pavan!

Let's start by checking with UX on this.

Helen, any thoughts about this feature?  Does it seem like a good idea?  If so, what UX seems best for it?  There's a mockup of one possible approach attached (attachment 8373993 [details]) based on old RDM.

Comment 45

[Pavan Karthik [:matrixisreal]]

The dropdown seems redundant as there are only two choices. What we can do is:
1. Have a single button for screenshots with two possible styles.
2. one for full screen screen shot and other for the just visible one.
3. the user need to click the button to toggle between the screenshot modes and so the button style also changes, notifying the user that the mode did changed.
4. [Optional] we may add a text dialogue to notify the user on each the click that the screenshot mode is changed. This is optional because the features is self explanatory to the user.
   I dont know how feasible is this. 
Let me know how to proceed if this sounds good.
thanks

Comment 46

[Helen V. Holmes (:helenvholmes) (:✨)]

There's a flag in Settings to screenshot either above the fold, or the entire page. While we might revisit this as it regards to RDM while working on multiviewport I'd rather not have a bug that states this particular approach as being our decision. My recommendation is not to change anything and WONTFIX this for the time being, Ryan.

Comment 47

[Florens Verschelde :fvsch]

(Reporter here.) Since:

1. the RDM UI has changed radically and is planned to change more in the future,
2. and there is a full-page screenshot button as an option in the devtools,

I think in the current context adding this kind of option or UI to the RDM would be a mistake. It's unclear right now if it would be more useful than costly (in design/dev time and in information overload). So I’m going to WONTFIX as suggested by Helen.

Generally speaking it might be a good idea to try and unify the built-in "page screenshot" tools in Firefox+DevTools. Currently there are at least 4:
1. In RDM (viewport)
2. In Inspector (Screenshot Node)
3. In main DevTools tab/command bar (optional button, full page)
4. With the Developer Toolbar (command line interface, can do everything)

I’d be in favor of removing (1) and (3), and providing a button or button-combobox for the navigation bar and main menu instead (so, a tool that can be added in Firefox's Customize mode).