Closed Bug 1023201 Opened 10 years ago Closed 10 years ago

Screenshots are missing 1px in height

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.1 S3 (29aug)
tracking-b2g backlog

People

(Reporter: gerard-majax, Assigned: jhobin)

References

Details

(Whiteboard: [systemsfe])

Attachments

(3 files, 1 obsolete file)

Taking screenshots on devices (Nexus S, Flame), the resulting pictures lacks one pixels in height:
 - on Nexus S, screenshot is 480x799
 - on Flame, screenshot is 480x853

I'm wondering how much this relates to bug 1023197 ...
From a visual perspective, is there a quick and dirty way to see the bug? That could help QA figure out to bisect this.
(In reply to Jason Smith [:jsmith] from comment #1)
> From a visual perspective, is there a quick and dirty way to see the bug?
> That could help QA figure out to bisect this.

Taking a screenshot, looking at its informations from the gallery, you will get the size of the picture.
No-Jun - Is this bug going to cause problems for your image comparison tool?
Flags: needinfo?(npark)
Minor regression, so let's not block on this.
blocking-b2g: 2.0? → backlog
(In reply to Jason Smith [:jsmith] from comment #3)
> No-Jun - Is this bug going to cause problems for your image comparison tool?

no- because both the master and slave will miss 1px.  thx for letting me know.
Flags: needinfo?(npark)
Jason, please note that I also noticed this behavior on device, but not with a reliable set of STRs. Once I remember was in Messages app, writing a SMS. In this case, I had keyboard started, and there was a slight tiny space between the bottom of the Messages app and the Keyboard app, where one could see the background wallpaper.
Sorry, forgot to add needinfo for comment 6. This may be a Gecko bug, but I have no idea who to ping.
Flags: needinfo?(jsmith)
Can we a screenshot of that bug?
Flags: needinfo?(jsmith)
Blocks: 1023197
(In reply to Jason Smith [:jsmith] from comment #8)
> Can we a screenshot of that bug?

Next time it reproduces, I'll try.
Attached image 2014-06-14-19-18-04.png
Jason, here is the screenshot. You can see that there is a tiny space between the keyboard and the SIM PIN dialog where you can see the background.
Flags: needinfo?(jsmith)
(In reply to Alexandre LISSY :gerard-majax from comment #10)
> Created attachment 8440313 [details]
> 2014-06-14-19-18-04.png
> 
> Jason, here is the screenshot. You can see that there is a tiny space
> between the keyboard and the SIM PIN dialog where you can see the background.

You can also see the screenshot is 480x853px :)
Given the new info from the screenshot, this is a visually obvious regression, so we should block.
blocking-b2g: backlog → 2.0?
Flags: needinfo?(jsmith)
blocking-b2g: 2.0? → 2.0+
Whiteboard: [systemsfe]
QA Contact: pcheng
The screenshot dimensions issue appears to be irrelevant to the SIM pin showing a 1px gap issue. The screenshot dimensions missing 1 px issue is NOT a regression; it appears on the earliest tinderbox Central Flame build, as well as on v121-2 base image only.

Attaching a screenshot taken from v121-2 base image. Note that its dimensions is 480x853 anywhere. You don't need to do any specific repro to get it to happen.

I have NOT been able to repro the SIM pin screen bug where it's showing a 1px gap between keyboard and confirmation on latest Flame master, earliest Flame master, and base image v121-2.

Removing regressionwindowwanted tag due to this is not a regression on Flame.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Not a regression - this is specific to the Flame's resolution, so we don't need to block on this.
blocking-b2g: 2.0+ → ---
Keywords: regression
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
blocking-b2g: --- → backlog
James, want to take a look?
This also reproduces on the v122 image. I will investigate this further.
Assignee: nobody → jhobin
(In reply to Jason Smith [:jsmith] from comment #14)
> Not a regression - this is specific to the Flame's resolution, so we don't
> need to block on this.

Are you sure this doesn't reproduce on any production device?
Flags: needinfo?(jsmith)
(In reply to Gregor Wagner [:gwagner] from comment #17)
> (In reply to Jason Smith [:jsmith] from comment #14)
> > Not a regression - this is specific to the Flame's resolution, so we don't
> > need to block on this.
> 
> Are you sure this doesn't reproduce on any production device?

This effect is produced by any resolution and device pixel ratio pairing which does not divide evenly. For example, the Flame's 854 height and 1.5 device pixel ratio results in a height of 569.333 CSS pixels. The screenshot code then uses the integer window.innerHeight to calculate the height it produces, which will end up being 569. The source of the 853 pixel height: floor(569 * 1.5) = 853.

I do not know which devices are considered production, but it appears many of them dodge this by having a one-to-one device pixel ratio.
Attached patch bug1023201.patch (obsolete) — Splinter Review
This fixes the screenshot dimensions bug by properly handling fractional CSS dimensions. This does not fix the line shown in 8440313, I believe that that is likely an unrelated issue caused by assuming that the device has integer CSS dimensions.

I have not tested whether this patch breaks existing automated tests. If someone could walk me through pushing to Try it would be excellent.
(In reply to Alexandre LISSY :gerard-majax from comment #10)
> Created attachment 8440313 [details]
> 2014-06-14-19-18-04.png
> 
> Jason, here is the screenshot. You can see that there is a tiny space
> between the keyboard and the SIM PIN dialog where you can see the background.

After seeing Naoki's bug similar to this, I actually think this might be a different problem than this bug, since this reproduces outside the scope of a screenshot. Can you open a new bug for this?

(In reply to Gregor Wagner [:gwagner] from comment #17)
> (In reply to Jason Smith [:jsmith] from comment #14)
> > Not a regression - this is specific to the Flame's resolution, so we don't
> > need to block on this.
> 
> Are you sure this doesn't reproduce on any production device?

I'll mark qawanted here to check Open C.
Flags: needinfo?(jsmith) → needinfo?(lissyx+mozillians)
Keywords: qawanted
(In reply to Jason Smith [:jsmith] from comment #20)
> (In reply to Alexandre LISSY :gerard-majax from comment #10)
> > Created attachment 8440313 [details]
> > 2014-06-14-19-18-04.png
> > 
> > Jason, here is the screenshot. You can see that there is a tiny space
> > between the keyboard and the SIM PIN dialog where you can see the background.
> 
> After seeing Naoki's bug similar to this, I actually think this might be a
> different problem than this bug, since this reproduces outside the scope of
> a screenshot. Can you open a new bug for this?

What do you want from me ? What other bug are you referring to ?
Flags: needinfo?(lissyx+mozillians)
(In reply to Alexandre LISSY :gerard-majax from comment #21)
> (In reply to Jason Smith [:jsmith] from comment #20)
> > (In reply to Alexandre LISSY :gerard-majax from comment #10)
> > > Created attachment 8440313 [details]
> > > 2014-06-14-19-18-04.png
> > > 
> > > Jason, here is the screenshot. You can see that there is a tiny space
> > > between the keyboard and the SIM PIN dialog where you can see the background.
> > 
> > After seeing Naoki's bug similar to this, I actually think this might be a
> > different problem than this bug, since this reproduces outside the scope of
> > a screenshot. Can you open a new bug for this?
> 
> What do you want from me ? What other bug are you referring to ?

The screenshot you attached in this comment is a different bug than this. bug 1035485 was where this was noticed - the screenshot attached here is actually a different problem than what this bug describes, since that bug actually happens outside the scope of a screenshot.
Flags: needinfo?(lissyx+mozillians)
(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Alexandre LISSY :gerard-majax from comment #21)
> > (In reply to Jason Smith [:jsmith] from comment #20)
> > > (In reply to Alexandre LISSY :gerard-majax from comment #10)
> > > > Created attachment 8440313 [details]
> > > > 2014-06-14-19-18-04.png
> > > > 
> > > > Jason, here is the screenshot. You can see that there is a tiny space
> > > > between the keyboard and the SIM PIN dialog where you can see the background.
> > > 
> > > After seeing Naoki's bug similar to this, I actually think this might be a
> > > different problem than this bug, since this reproduces outside the scope of
> > > a screenshot. Can you open a new bug for this?
> > 
> > What do you want from me ? What other bug are you referring to ?
> 
> The screenshot you attached in this comment is a different bug than this.
> bug 1035485 was where this was noticed - the screenshot attached here is
> actually a different problem than what this bug describes, since that bug
> actually happens outside the scope of a screenshot.

I'm asking for a new bug to be filed here for that issue.

Also - can't do the QA Wanted request anymore since the issue that was thought to be used to reproduce this issue isn't actually what this bug talks about.
Keywords: qawanted
(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Alexandre LISSY :gerard-majax from comment #21)
> > (In reply to Jason Smith [:jsmith] from comment #20)
> > > (In reply to Alexandre LISSY :gerard-majax from comment #10)
> > > > Created attachment 8440313 [details]
> > > > 2014-06-14-19-18-04.png
> > > > 
> > > > Jason, here is the screenshot. You can see that there is a tiny space
> > > > between the keyboard and the SIM PIN dialog where you can see the background.
> > > 
> > > After seeing Naoki's bug similar to this, I actually think this might be a
> > > different problem than this bug, since this reproduces outside the scope of
> > > a screenshot. Can you open a new bug for this?
> > 
> > What do you want from me ? What other bug are you referring to ?
> 
> The screenshot you attached in this comment is a different bug than this.
> bug 1035485 was where this was noticed - the screenshot attached here is
> actually a different problem than what this bug describes, since that bug
> actually happens outside the scope of a screenshot.

I don't get your point. This bug has always been about the height of the screenshot. Comment 10 was about this space but as you said, this may have been bug 1035485, and I only evocated it as maybe another manifestation. But given bug 1035485, that may have been unrelated.

That does not change anything about this bug: any screenshot expose a -1px height. Better, James proposed a fix in comment 19.
Flags: needinfo?(lissyx+mozillians)
(In reply to Alexandre LISSY :gerard-majax from comment #24)
> (In reply to Jason Smith [:jsmith] from comment #22)
> > (In reply to Alexandre LISSY :gerard-majax from comment #21)
> > > (In reply to Jason Smith [:jsmith] from comment #20)
> > > > (In reply to Alexandre LISSY :gerard-majax from comment #10)
> > > > > Created attachment 8440313 [details]
> > > > > 2014-06-14-19-18-04.png
> > > > > 
> > > > > Jason, here is the screenshot. You can see that there is a tiny space
> > > > > between the keyboard and the SIM PIN dialog where you can see the background.
> > > > 
> > > > After seeing Naoki's bug similar to this, I actually think this might be a
> > > > different problem than this bug, since this reproduces outside the scope of
> > > > a screenshot. Can you open a new bug for this?
> > > 
> > > What do you want from me ? What other bug are you referring to ?
> > 
> > The screenshot you attached in this comment is a different bug than this.
> > bug 1035485 was where this was noticed - the screenshot attached here is
> > actually a different problem than what this bug describes, since that bug
> > actually happens outside the scope of a screenshot.
> 
> I don't get your point. This bug has always been about the height of the
> screenshot. Comment 10 was about this space but as you said, this may have
> been bug 1035485, and I only evocated it as maybe another manifestation. But
> given bug 1035485, that may have been unrelated.
> 
> That does not change anything about this bug: any screenshot expose a -1px
> height. Better, James proposed a fix in comment 19.

Right - but you should file a separate bug for what's in that screenshot in comment 10, as that's a different problem unrelated to this bug.
Flags: needinfo?(lissyx+mozillians)
Okay, sorry, your statement was unclear to me. After looking at bug 1035485 you duped, I did not noticed this.
Flags: needinfo?(lissyx+mozillians)
The Try build results appear to be green: https://tbpl.mozilla.org/?tree=Try&rev=2ab8e69e3951
What are you waiting for to ask a review ? :)
Flags: needinfo?(jhobin)
Comment on attachment 8452755 [details] [diff] [review]
bug1023201.patch

A reminder :)
Attachment #8452755 - Flags: review?(lissyx+mozillians)
Flags: needinfo?(jhobin)
Comment on attachment 8452755 [details] [diff] [review]
bug1023201.patch

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

I'm not a peer for this :(
Attachment #8452755 - Flags: review?(lissyx+mozillians)
Attachment #8452755 - Flags: review?(fabrice)
Comment on attachment 8452755 [details] [diff] [review]
bug1023201.patch

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

|var| everywhere... sigh.

::: b2g/chrome/content/shell.js
@@ +963,5 @@
> +      var width = docRect.width;
> +      var height = docRect.height;
> +
> +      // Convert width and height from CSS pixels (potentially fractional)
> +      // to device pixels (integer)

nit: full stop at the end of the comment.
Attachment #8452755 - Flags: review?(fabrice) → review+
Attached patch bug1023201.patchSplinter Review
(In reply to jhobin from comment #32)
> Created attachment 8475434 [details] [diff] [review]
> bug1023201.patch

This adds the period to the end of the comment and (more importantly) is the patch off which the latest try https://tbpl.mozilla.org/?tree=Try&rev=725a1dde45d0 is based. The previous Try was no longer available due to some tree closure database wipe thing.
Keywords: checkin-needed
(In reply to jhobin from comment #33)
> (In reply to jhobin from comment #32)
> > Created attachment 8475434 [details] [diff] [review]
> > bug1023201.patch
> 
> This adds the period to the end of the comment and (more importantly) is the
> patch off which the latest try
> https://tbpl.mozilla.org/?tree=Try&rev=725a1dde45d0 is based. The previous
> Try was no longer available due to some tree closure database wipe thing.

FYI, the push data itself was removed, but the job results remain available for it for 30 days. You don't need a new Try push just because of the reset.
Attachment #8452755 - Attachment is obsolete: true
https://hg.mozilla.org/integration/b2g-inbound/rev/6a740a2a465c

But yes, in this case, you hit the >30d expiration date on your older Try push. Had nothing to do with the reset. Also, please make sure your future patches contain proper hg commit information, including author and a useful commit message. Thanks.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6a740a2a465c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.