Closed
Bug 1023201
Opened 10 years ago
Closed 10 years ago
Screenshots are missing 1px in height
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(tracking-b2g:backlog)
People
(Reporter: gerard-majax, Assigned: jhobin)
References
Details
(Whiteboard: [systemsfe])
Attachments
(3 files, 1 obsolete file)
40.12 KB,
image/png
|
Details | |
800.89 KB,
image/png
|
Details | |
1.12 KB,
patch
|
Details | Diff | Splinter Review |
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 ...
Comment 1•10 years ago
|
||
From a visual perspective, is there a quick and dirty way to see the bug? That could help QA figure out to bisect this.
Reporter | ||
Comment 2•10 years ago
|
||
(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.
Comment 3•10 years ago
|
||
No-Jun - Is this bug going to cause problems for your image comparison tool?
Flags: needinfo?(npark)
Comment 5•10 years ago
|
||
(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)
Reporter | ||
Comment 6•10 years ago
|
||
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.
Reporter | ||
Comment 7•10 years ago
|
||
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)
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #8) > Can we a screenshot of that bug? Next time it reproduces, I'll try.
Reporter | ||
Comment 10•10 years ago
|
||
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)
Reporter | ||
Comment 11•10 years ago
|
||
(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 :)
Comment 12•10 years ago
|
||
Given the new info from the screenshot, this is a visually obvious regression, so we should block.
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Updated•10 years ago
|
Whiteboard: [systemsfe]
Updated•10 years ago
|
QA Contact: pcheng
Comment 13•10 years ago
|
||
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.
Updated•10 years ago
|
Comment 14•10 years ago
|
||
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
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Updated•10 years ago
|
blocking-b2g: --- → backlog
Comment 15•10 years ago
|
||
James, want to take a look?
Assignee | ||
Comment 16•10 years ago
|
||
This also reproduces on the v122 image. I will investigate this further.
Assignee: nobody → jhobin
Comment 17•10 years ago
|
||
(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)
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
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.
Comment 20•10 years ago
|
||
(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
Reporter | ||
Comment 21•10 years ago
|
||
(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)
Comment 22•10 years ago
|
||
(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)
Comment 23•10 years ago
|
||
(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
Reporter | ||
Comment 24•10 years ago
|
||
(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)
Comment 25•10 years ago
|
||
(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)
Reporter | ||
Comment 26•10 years ago
|
||
Okay, sorry, your statement was unclear to me. After looking at bug 1035485 you duped, I did not noticed this.
Flags: needinfo?(lissyx+mozillians)
Assignee | ||
Comment 27•10 years ago
|
||
The Try build results appear to be green: https://tbpl.mozilla.org/?tree=Try&rev=2ab8e69e3951
Reporter | ||
Comment 28•10 years ago
|
||
What are you waiting for to ask a review ? :)
Flags: needinfo?(jhobin)
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 8452755 [details] [diff] [review] bug1023201.patch A reminder :)
Attachment #8452755 -
Flags: review?(lissyx+mozillians)
Flags: needinfo?(jhobin)
Reporter | ||
Comment 30•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8452755 -
Flags: review?(fabrice)
Comment 31•10 years ago
|
||
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+
Assignee | ||
Comment 32•10 years ago
|
||
Assignee | ||
Comment 33•10 years ago
|
||
(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
Comment 34•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8452755 -
Attachment is obsolete: true
Comment 35•10 years ago
|
||
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)
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•