Make it clear that we show gecko's resolution, not device's resolution

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: WebIDE
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: gerard, Assigned: wldcordeiro, Mentored)

Tracking

unspecified
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [good first bug][lang=html])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Running a B2G with Gecko 28.0a1 build of today on a HTC Desire Z, connecting with App Manager from Nightly on the device, I get this resolution: « Taille de l'appareil : 320 × 533 (254 PPP) »

The screen is 480x800, 252 dpi, as specified on http://www.gsmarena.com/htc_desire_z-3421.php

Updated

5 years ago
Component: General → Developer Tools: App Manager
Product: Firefox OS → Firefox

Comment 1

5 years ago
We show Gecko's resolution. Which, I think, is what matter for app developers.
(Reporter)

Comment 2

5 years ago
(In reply to Paul Rouget [:paul] from comment #1)
> We show Gecko's resolution. Which, I think, is what matter for app
> developers.

Fine by me then. Maybe the label is a bit misleading ?
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID

Updated

5 years ago
Duplicate of this bug: 934736
(In reply to Paul Rouget [:paul] from comment #1)
> We show Gecko's resolution. Which, I think, is what matter for app
> developers.

It's pretty unexpected behavior for someone installing a packaged app on their 1080p Android device via the App Manager (the only current user flow), though.

Can you clarify what you mean by "Gecko's resolution"? My device is rendering 1px lines as a single device pixel, not as 2+ pixels, so it would seem that I have the full screen resolution, not one quarter of it, to work with…
Flags: needinfo?(paul)
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Summary: [B2G][AppManager] Invalid device resolution detected on HTC Desire Z → [B2G][AppManager] Invalid device resolution detected on HTC Desire Z running B2G, HTC One running Firefox

Comment 5

5 years ago
(In reply to Richard Newman [:rnewman] from comment #4)
> (In reply to Paul Rouget [:paul] from comment #1)
> > We show Gecko's resolution. Which, I think, is what matter for app
> > developers.
> 
> It's pretty unexpected behavior for someone installing a packaged app on
> their 1080p Android device via the App Manager (the only current user flow),
> though.
>
> Can you clarify what you mean by "Gecko's resolution"? My device is
> rendering 1px lines as a single device pixel, not as 2+ pixels, so it would
> seem that I have the full screen resolution, not one quarter of it, to work
> with…

On the web, a pixel is not a pixel:

HTC Desire Z: gecko exposes 320x533. The screen (hardware) size is 480x800.

If the developer writes: <div style="width:320px">, the <div> will cover the whole screen.


I understand this is confusing. Maybe we should show 2 sections here.
Flags: needinfo?(paul)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: [B2G][AppManager] Invalid device resolution detected on HTC Desire Z running B2G, HTC One running Firefox → [B2G][AppManager] Make it clear that we show gecko's resolution, not device's resolution

Comment 6

5 years ago
this really is confusing, as far as I understand gecko's resolution is a scaled factor of the actual device resolution, please correct me if I'm wrong

I actually had a conversation with the geekphone support team which could have been avoided if there would be a document on mdn with further explanation

Comment 8

5 years ago
needinfo triggered
Flags: needinfo?(paul)

Comment 9

5 years ago
(In reply to Soelen from comment #6)
> this really is confusing

What is? Looking at your link I don't see anything related to the app manager.

I think your confused about how pixels work on the web (which is not exactly related to this, which is about the App Manager).

> as far as I understand gecko's resolution is a
> scaled factor of the actual device resolution, please correct me if I'm wrong

This is not just gecko. This is true for all the html engines.
This is how the web works.

> I actually had a conversation with the geekphone support team which could
> have been avoided if there would be a document on mdn with further
> explanation

https://developer.mozilla.org/en-US/docs/Web/CSS/length
https://developer.mozilla.org/en-US/docs/Mozilla/Mobile/Viewport_meta_tag#A_pixel_is_not_a_pixel
http://www.quirksmode.org/blog/archives/2010/04/a_pixel_is_not.html
Flags: needinfo?(paul)

Comment 10

5 years ago
>Looking at your link I don't see anything related to the app manager.

I guess you are right, sorry for posting on the wrong place. irc.mozilla.org#b2g thought this might have something to do with my issue and I had no idea since this truly is a new topc for me.

thanks for the links, those where indeed quite useful :)
Marking as good first bug.  Seems like we could add a tooltip and / or some links in the App Manager to the info Paul referenced above.
Whiteboard: [good first bug][lang=html][mentor=jryans]
Mentor: jryans
Whiteboard: [good first bug][lang=html][mentor=jryans] → [good first bug][lang=html]
Hello!
A newbie here.
I'd like to work on this bug.
(In reply to Ranveer Aggarwal [:ranveer] from comment #12)
> Hello!
> A newbie here.
> I'd like to work on this bug.

Great!  Take a look at our hacking page[1] to get started with the code base.

We'll want to make this change in WebIDE's runtime details page[2], where the width and height items are shown.  The details are added by looping over all the items generically, so we may need to special case width and height to add more details for those items.

Feel free to attempt whatever design seems most helpful to indicate the issue here.  Maybe you could link to ppk's article[3] and put in note in the tooltip, but up to you.

If you have questions, you can ping me on IRC, or set the "need info" block on this bug to my email.

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/runtimedetails.js#48
[3]: http://www.quirksmode.org/blog/archives/2013/11/screenwidth_is.html
I hit this bug today with the Flame. A larger problem is that screenshots taken of the Flame with the Screenshot tool are in the lower, non-native resolution, whereas on-device screenshots are full resolution. Not sure if I should open a separate bug for that.

Comment 15

4 years ago
(In reply to Thomas Daede from comment #14)
> I hit this bug today with the Flame. A larger problem is that screenshots
> taken of the Flame with the Screenshot tool are in the lower, non-native
> resolution, whereas on-device screenshots are full resolution. Not sure if I
> should open a separate bug for that.

bug 1021547
(In reply to J. Ryan Stinnett [:jryans] from comment #13)
> Great!  Take a look at our hacking page[1] to get started with the code base.
Yep! I got the build up and running. Used WebIDE to Simulate FXOS v2.1.
> 
> We'll want to make this change in WebIDE's runtime details page[2], where
> the width and height items are shown.  The details are added by looping over
> all the items generically, so we may need to special case width and height
> to add more details for those items.
As far as I understand, I need to add a separate condition in case `name=width`.
> 
> Feel free to attempt whatever design seems most helpful to indicate the
> issue here.  Maybe you could link to ppk's article[3] and put in note in the
> tooltip, but up to you.
Could you guide me more on how to add this element? I'm a bit confused here.
> 
> If you have questions, you can ping me on IRC, or set the "need info" block
> on this bug to my email.
> 
> [1]: https://wiki.mozilla.org/DevTools/Hacking
> [2]:
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/
> content/runtimedetails.js#48
> [3]: http://www.quirksmode.org/blog/archives/2013/11/screenwidth_is.html

Comment 17

4 years ago
So, I believe the only thing you need to do is, in toolkit/devtools/server/actors/device.js, replace:

> width: win.screen.width,
> height: win.screen.height

By something like that:

> CSS_width: win.screen.width,
> CSS_height: win.screen.height
> physical_width: win.screen.width * win.devicePixelRatio,
> physical_height: win.screen.height * win.devicePixelRatio

And in browser/devtools/app-manager/content/device.xhtml, replace

> device.description.width","device.description.height"

with:

> device.description.physical_width","device.description.physical_height"

That should do the trick.
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #17)
> So, I believe the only thing you need to do is, in
> toolkit/devtools/server/actors/device.js, replace:
> 
> > width: win.screen.width,
> > height: win.screen.height
> 
> By something like that:
> 
> > CSS_width: win.screen.width,
> > CSS_height: win.screen.height
> > physical_width: win.screen.width * win.devicePixelRatio,
> > physical_height: win.screen.height * win.devicePixelRatio
> 
> And in browser/devtools/app-manager/content/device.xhtml, replace
> 
> > device.description.width","device.description.height"
> 
> with:
> 
> > device.description.physical_width","device.description.physical_height"
> 
> That should do the trick.

Didn't work. At least for the simulator. Would it work for a physical device?

Comment 19

4 years ago
(In reply to Ranveer Aggarwal [:ranWeird] from comment #18)
> Didn't work. At least for the simulator. Would it work for a physical device?

Did you rebuild the simulator? And also, it would only work if your display has a different pixel density different to 1.
I did rebuild the simulator.
Here's the screenshot of what it shows now: http://imgur.com/ooyjE1G

Comment 21

4 years ago
(In reply to Ranveer Aggarwal [:ranWeird] from comment #20)
> I did rebuild the simulator.
> Here's the screenshot of what it shows now: http://imgur.com/ooyjE1G

Did you rebuild Firefox too?
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #21)
> Did you rebuild Firefox too?
Yep, I rebuilt everything.
(In reply to Ranveer Aggarwal [:ranWeird] from comment #20)
> I did rebuild the simulator.
> Here's the screenshot of what it shows now: http://imgur.com/ooyjE1G

Can you describe your simulator build steps in more detail?

Did you run something like "mach build && mach pacakge" with a b2g-desktop mozconfig, and then install the resulting simulator XPI?
(In reply to J. Ryan Stinnett [:jryans] from comment #23)
> Can you describe your simulator build steps in more detail?
> 
> Did you run something like "mach build && mach pacakge" with a b2g-desktop
> mozconfig, and then install the resulting simulator XPI?

Actually, now I'm unable to build at all. I did file a bug https://bugzilla.mozilla.org/show_bug.cgi?id=1070721, but haven't been able to get anything.

Updated

3 years ago
Flags: in-testsuite-
Flags: in-qa-testsuite?

Updated

3 years ago
Flags: in-testsuite-
Flags: in-qa-testsuite?
Summary: [B2G][AppManager] Make it clear that we show gecko's resolution, not device's resolution → Make it clear that we show gecko's resolution, not device's resolution

Comment 25

3 years ago
Hi, I would like to fix this bug. This is my first bug. Can you please guide me on how to go about it
(Assignee)

Comment 26

2 years ago
If no one is actively working on this I'd like to take a look at it.
Flags: needinfo?(jryans)
Sure, feel free to give it a shot!

Here's a workflow you can use without building anything: 

https://developer.mozilla.org/en-US/docs/Tools/Contributing/Contribute_on_nightly

Comment 17 roughly describes the changes we need, except the files have moved.  It would now be:

https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/device.js

It should appear in WebIDE's runtime info:

https://developer.mozilla.org/en-US/docs/Tools/WebIDE/The_runtime_menu#Runtime_info
Flags: needinfo?(jryans)
(Assignee)

Comment 28

2 years ago
Thank you jryans, So I read through comment 17 and he references that file as well as an xhtml one that has also moved and I can't seem to locate. Additionally I'm not sure what I'm supposed to do with the `physicalHeight` and `physicalWidth` variables unlike the other two and eslint complains they're unused.

Here's a diff view, could you explain what I could do with those?
https://github.com/wldcordeiro/gecko-dev/pull/2/files

Otherwise this seems pretty straightforward. :)

Note: I know my commit message needs to be a certain format, I just did that to have a nice to view diff that's linkable.
(In reply to Wellington Cordeiro from comment #28)
> Thank you jryans, So I read through comment 17 and he references that file
> as well as an xhtml one that has also moved and I can't seem to locate.
> Additionally I'm not sure what I'm supposed to do with the `physicalHeight`
> and `physicalWidth` variables unlike the other two and eslint complains
> they're unused.
> 
> Here's a diff view, could you explain what I could do with those?
> https://github.com/wldcordeiro/gecko-dev/pull/2/files
> 
> Otherwise this seems pretty straightforward. :)
> 
> Note: I know my commit message needs to be a certain format, I just did that
> to have a nice to view diff that's linkable.

This bug is not actually about the screenshot function, it's about the device description data from `getDescription`.  It looks like the interesting part of that method was moved, so we actually want to change the system info over here:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/system.js#164-165

When you attach a patch to the bug, it provides a diff view.  Also it's best to use the attachment flags to request feedback or review to be sure the patch is not missed in the torrent of bug mail.
(Assignee)

Comment 30

2 years ago
I see, thanks jryans. I was able to find and make the changes in the js file however I still don't know what became of "browser/devtools/app-manager/content/device.xhtml" that Paul Roget referred to.
Flags: needinfo?(jryans)
(Assignee)

Comment 31

2 years ago
Created attachment 8749461 [details] [diff] [review]
Bug933170.patch
Attachment #8749461 - Flags: review?(jryans)
Attachment #8749461 - Flags: feedback?(jryans)
(In reply to Wellington Cordeiro from comment #30)
> I see, thanks jryans. I was able to find and make the changes in the js file
> however I still don't know what became of
> "browser/devtools/app-manager/content/device.xhtml" that Paul Roget referred
> to.

Sorry, I should have been more specific about this!  The App Manager tool was removed, so these files are gone now.  However, many of the functions were copied to WebIDE, which is why you can test this behavior in WebIDE.  The design of it in WebIDE doesn't seem to need updated the UI because it just shows all the properties it receives.
Flags: needinfo?(jryans)
Comment on attachment 8749461 [details] [diff] [review]
Bug933170.patch

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

This makes sense overall, I think!  Thanks for taking a look at this. :)

For the future, only one of the patch flags (review or feedback) is needed.  feedback is more like "I am not sure about this, but wanted to get your opinion", while review is "here is something I believe is ready to land".

Seems like just a small change is needed.

::: devtools/shared/system.js
@@ +169,5 @@
>      channel: AppConstants.MOZ_UPDATE_CHANNEL,
>  
>      dpi,
>      useragent,
> +    cssWidth,

This data is exchanged between the DevTools client and server, and we try our best to preserve compatibility, so that a client can still connect to older servers.

For this specific case, it seems safest to leave the existing key names the way they were and only add new ones.
Attachment #8749461 - Flags: review?(jryans)
Attachment #8749461 - Flags: feedback?(jryans)
Attachment #8749461 - Flags: feedback+
(Assignee)

Comment 34

2 years ago
Created attachment 8749758 [details]
Distinct Heights and Widths
(Assignee)

Comment 35

2 years ago
Ah okay, thanks for all the help jryans! I think my patch should actually be good then. I've attached a screenshot as well.
(Assignee)

Comment 36

2 years ago
clarification then. Instead of `width` -> `cssWidth` and same for height, we leave that as is and add only the `physicalWidth`, `physicalHeight` keys?
Flags: needinfo?(jryans)
(In reply to Wellington Cordeiro from comment #36)
> clarification then. Instead of `width` -> `cssWidth` and same for height, we
> leave that as is and add only the `physicalWidth`, `physicalHeight` keys?

Yes, that sounds right to me.
Flags: needinfo?(jryans)
(Assignee)

Comment 38

2 years ago
Created attachment 8749768 [details] [diff] [review]
Bug933170.patch
Attachment #8749461 - Attachment is obsolete: true
Attachment #8749768 - Flags: review?(jryans)
(Assignee)

Comment 39

2 years ago
Created attachment 8749769 [details]
screenshot after review
Comment on attachment 8749768 [details] [diff] [review]
Bug933170.patch

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

This seems good to me, thanks for working on it!

I've pushed this work to the try server for you:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e57d0dd2e6
Attachment #8749768 - Flags: review?(jryans) → review+
Assuming the try run looks green, feel free to set the "checkin-needed" keyword on this bug.
(Assignee)

Comment 42

2 years ago
J. Ryan, it looks like I can't do that? I don't see that as an editable field.
Flags: needinfo?(jryans)
(In reply to Wellington Cordeiro from comment #42)
> J. Ryan, it looks like I can't do that? I don't see that as an editable
> field.

Ah right!  I have updated your Bugzilla permissions, so you should be able to do it now! :)
Flags: needinfo?(jryans)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 45

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4da268d843df
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee: nobody → wellington
You need to log in before you can comment on or make changes to this bug.