Closed Bug 961832 Opened 10 years ago Closed 10 years ago

GCLI screenshot shows fixed position element in wrong position

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Product:
DevTools Graveyard
Component:
Graphic Commandline and Toolbar
Version:
26 Branch
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox33 verified)

Status:
RESOLVED FIXED
Milestone:
Firefox 33
Tracking Flags:
Tracking Status
firefox33 --- verified

People

(Reporter: fletch, Assigned: sblin)

References

Details

Attachments

(2 files, 3 obsolete files)

firefox_fixed_position_screenshot_bug.png
14.25 KB, image/png
Details
screenshot-fullpage-fix.diff
2.27 KB, patch
pbro
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

Create a fixed position "menu bar", scroll the page down, and take a screenshot using GCLI

My test document:

<!DOCTYPE html>
<html>
	<head>
		<meta charset="utf-8">
		<style>
			body {
				margin-top: 40px;
			}
			.menu-bar {
				width: 100%;
				height: 40px;
				position: fixed;
				top: 0;
				left: 0;
				background-color: black;
				color: white;
			}
			.content {
				width: 900px;
				height: 1200px;
				border: 1px solid red;
				margin-left: auto;
				margin-right: auto;
			}
		</style>
	</head>
	<body>
		<div class="menu-bar">
			Menu Bar
		</div>
		<div class="content">
			Content
		</div>
	</body>
</html>


Actual results:

The fixed position "menu bar" appears halfway down the page on the screenshot, even though it appeared at the top of the page in browser when the screenshot was taken


Expected results:

The "menu bar" should have stayed on the top of the page in the screenshot, exactly like it does when the browser renders it
Could not reproduce using Firefox 26 and latest Aurora. I made a screenshot using the Developer Toolbar (Shift+F2 and type 'screenshot'). The menu bar stays on top of the page.
Greg can you please see if this reproduces for you in safe mode or with a new profile?
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles
Flags: needinfo?(fletch)
I was able to reproduce the issue both in safe mode as well as with a new profile. I think I missed a key detail on being able to reproduce it though, it must be a --fullpage screnshot.
Flags: needinfo?(fletch)
Can reproduce on Linux Firefox 33 both by doing a --fullpage via commandline and using developer toolbox button.
OS: Windows 7 → All
Attached patch screenshot-fullpage-fix.patch (obsolete) — — Splinter Review 

        Attachment #8446764 -
        Flags: review?(pbrosset)

        Attachment #8446764 -
        Flags: review?(jwalker)

    
    

    
  
I propose one solution :
If we request a fullscreen screenshot, the scrollbar goes to 0,0 just for the screenshot and repass to the last known position.

An other solution is to move fixed elements to the basic position, but I think it is not the better solution.

    
    

    
  
Comment on attachment 8446764 [details] [diff] [review]
screenshot-fullpage-fix.patch

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

This seems to work fine, of course it makes the scrollbar flicker, but I don't think that's a big deal.
I prefer this solution to the other one you described.
The only thing is we're scrolling in all cases, even if there's no fixed positioned elements (or if there are, but in iframes).
This is probably not a big deal unless the page is polling for the scroll position to dynamically change the page, in which case the screenshot wouldn't look like what the user is seeing. I guess these cases might be rarer than the case of someone taking a screenshot of a page with a position:fixed menubar.

I've made a few minor comments and one about not scrolling if there's a node. Let me know what you think.

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +106,5 @@
>          let width;
>          let height;
>          let div = document.createElementNS("http://www.w3.org/1999/xhtml", "div");
> +        let currentX = 0;
> +        let currentY = 0;

nit: Can you add an empty line here, as there was one before to separate the if part out.

@@ +121,5 @@
>              left = rect.left;
>              width = rect.width;
>              height = rect.height;
>            }
> +        } else {          

nit: trailing whitespaces

@@ +129,5 @@
> +          } else {
> +            let lh = new LayoutHelpers(window);
> +            let rect = lh.getRect(node, window);
> +            currentY = rect.top;
> +            currentX = rect.left;

Do we need to do anything special when taking a screenshot of a node?
position:fixed is always relative to the viewport anyway. I think you should only scroll to 0,0 if there is no node.

@@ +180,5 @@
>            }
>            throw new Task.Result(div);
>          }
>  
> +        if(fullpage)        

nit: trailing whitespaces, also, we normally enclose all statement bodies in {}

        Attachment #8446764 -
        Flags: review?(pbrosset) → review+

    
    

    
  
I prefer this solution too.
Thank you for the nit, I will change my patch and reattach.

"Do we need to do anything special when taking a screenshot of a node?
position:fixed is always relative to the viewport anyway. I think you should only scroll to 0,0 if there is no node." -> I think you are right.

    
    

    
  
Comment on attachment 8446764 [details] [diff] [review]
screenshot-fullpage-fix.patch

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

Thanks Sébastien. It looks like Patrick is doing a good job - happy to let him carry on doing that :)

        Attachment #8446764 -
        Flags: review?(jwalker)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        screenshot-fullpage-fix.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8447145 -
        Flags: review+

        Attachment #8447145 -
        Flags: checkin?

    
    

    
  
I trail whitespaces and add {} and add the empty line.
But I can't be assigned or modify keywords (add checkin-needed keyword) because I am level 0.
So if somebody can add checkin-needed keyword :).
Flags: needinfo?(pbrosset)

        Attachment #8446764 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8447145 [details] [diff] [review]
screenshot-fullpage-fix.patch

In the future, please just use the checkin-needed bug keyword. It plays more nicely with our bug marking tools :)

        Attachment #8447145 -
        Flags: checkin?
Flags: needinfo?(pbrosset)

    
    

    
  
Hrm, need to find someone who can give you editbugs privs so you can set checkin-needed yourself :)

Also, can we please run this through Try to make sure it won't cause test failures when it lands? Patrick should be able to assist with that.

Thanks for the patch!
Flags: needinfo?(pbrosset)
Assignee: nobody → amarok
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

    
    

    
  
Comment on attachment 8447145 [details] [diff] [review]
screenshot-fullpage-fix.patch

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

I wanted to push this patch to try but it doesn't apply cleanly on the latest fx-team update.
Could you rebase the patch?

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +122,5 @@
>              left = rect.left;
>              width = rect.width;
>              height = rect.height;
>            }
>          } else {         

I can still see the trailing whitespaces after '} else {'

@@ +127,5 @@
> +          if (!node) {
> +            currentX = window.scrollX;
> +            currentY = window.scrollY;
> +          } else {
> +            let lh = new LayoutHelpers(window);

Didn't you agree to remove that part?

@@ +180,5 @@
>            }
>            throw new Task.Result(div);
>          }
>  
> +        if(fullpage) {       

I can still see the trailing whitespaces after 'if(fullpage) {'

        Attachment #8447145 -
        Flags: review+
Flags: needinfo?(pbrosset)

    
    

    
  
Oh, sorry I am french and I didn't understand trailing whitespaces. My fault. (I understand this time).

I test with removing the part
"if (!node) {
> +            currentX = window.scrollX;
> +            currentY = window.scrollY;
> +          } else {"
->
"if (!node) {
> +          } else {"

And when I test with my website, after the screenshot, the scrollbar go to 0,0 but no return to the user position when he takes the screenshot (You can test with the HTML page in the first comment)

    
    

    
  
(In reply to Sébastien Blin [:sblin] [:amarok] from comment #14)
> Oh, sorry I am french and I didn't understand trailing whitespaces.

Don't apologise for being French, and certainly not to Patrick :)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        screenshot-fullscreen-fix.diff (obsolete)
        — 
          — Splinter Review
      

    


  
Can you test with this patch ?

        Attachment #8447145 -
        Attachment is obsolete: true

        Attachment #8447613 -
        Flags: review+
Flags: needinfo?(pbrosset)

    
    

    
  
Comment on attachment 8447613 [details] [diff] [review]
screenshot-fullscreen-fix.diff

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

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +130,5 @@
> +          } else {
> +            let lh = new LayoutHelpers(window);
> +            let rect = lh.getRect(node, window);
> +            currentY = rect.top;
> +            currentX = rect.left;

I don’t think this code does anything sensible or is intended.

It appears in `--fullpage` branch and `node` is truthy only when `--selector` is specified, meaning that at this point both `--fullscreen` and `--selector` are specified. `--fullpage` and `--selector` sound mutually exclusive to me.

Regardless of that, the code will take the shot of full page anyway, but if --selector is set, after the screenshot the page will scroll to the element specified to --selector and not to the location before initiating the screenshot, which doesn’t appear to be the intended behaviour.

    
    

    
  
Comment on attachment 8447613 [details] [diff] [review]
screenshot-fullscreen-fix.diff

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

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +130,5 @@
> +          } else {
> +            let lh = new LayoutHelpers(window);
> +            let rect = lh.getRect(node, window);
> +            currentY = rect.top;
> +            currentX = rect.left;

I agree with Simonas, calling the screenshot command with both --fullpage AND --selector doesn't make sense and before your patch, the code used to simply ignore the 'node' variable in this branch. So I suggest we do the same here, just don't test for 'node' and scroll to 0,0 as you're doing.

@@ +183,5 @@
>          }
>  
> +        if (fullpage) {
> +          window.scrollTo(currentX, currentY);
> +        }

I think this would look better if this part was closer to the first 'scrollTo'. The function is pretty long and it's easy to loose track of context when reading.
It's not a big deal but since you'll be changing the patch anyway, I would suggest move these 3 lines up a bit, to just after 'let data = canvas.toDataURL("image/png", "");' perhaps.

        Attachment #8447613 -
        Flags: review+

    
    

    
  
Clearing the needinfo flag.
Oh and, I've tested, the patch works fine for me on the sample page provided in comment 0.
Flags: needinfo?(pbrosset)

    
    

    
  


  
I clear the patch. 
I init currentX & currentY, remove the if and move the scrollTo

        Attachment #8447613 -
        Attachment is obsolete: true

        Attachment #8448034 -
        Flags: review?(pbrosset)

    
    

    
  
Comment on attachment 8448034 [details] [diff] [review]
screenshot-fullpage-fix.diff

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

Looks good to me. Thanks for the patch Sébastien!
Here is a pending try build: https://tbpl.mozilla.org/?tree=Try&rev=b0cc6b44de7c
Let's land the patch as soon as results are green.

        Attachment #8448034 -
        Flags: review?(pbrosset) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0df20d946272
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33

    
    

    
  
bug resolved
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:33.0) Gecko/20100101 Firefox/33.0 buildID	20140716030202

[bugday-20140716]

http://imgur.com/JdXPO46
QA Whiteboard: [bugday-20140716]

          status-firefox33:
          --- → verified
Component: Untriaged → Layout
Product: Firefox → Core
Target Milestone: Firefox 33 → mozilla33
Component: Layout → Developer Tools: Graphic Commandline and Toolbar
Product: Core → Firefox
Target Milestone: mozilla33 → ---
Target Milestone: --- → Firefox 33

    
    

    
  
taken with GCLI: http://imgur.com/k82wmGr
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:33.0) Gecko/20100101 Firefox/33.0 buildID	20140716030202
[bugday-20140716]

    
  
Product: Firefox → DevTools

    
  
Product: DevTools → DevTools Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: