Desktop mode should defeat responsive design

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kats, Assigned: mantaroh, Mentored)

Tracking

unspecified
Firefox 39
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [lang=js][lang=c++])

Attachments

(2 attachments, 5 obsolete attachments)

Posted patch PatchSplinter Review
Some sites (e.g. treeherder.mozilla.org) use responsive design but are actually quite hard to use in this mode. I would much rather have the ability to force a desktop-sized viewport when rendering the page. This seems like it should be rolled into "request desktop site".

The attached patch is a start. If we want this there's still an open question as to whether or not we want font inflation enabled in desktop mode (the patch will leave it enabled).
Comment on attachment 8531347 [details] [diff] [review]
Patch

The idea has merit. This would also defeat any viewport meta tag, which probably makes sense for Desktop Mode.
Attachment #8531347 - Flags: feedback?(mark.finkle) → feedback+
Assignee: bugmail.mozilla → nobody
Mentor: bugmail.mozilla
Whiteboard: [lang=js][lang=c++]
Hi Kartikaya,i want to help in this bug .
Can you please guide to what's still left to be done?
Hi surabhi, I see that the previous bugs you've worked on have all been in Java code. Are you comfortable working in C++ code as well? The remaining part of this bug is mostly in C++ code, we would need to add some mechanism to disable font inflation in tabs that are using Desktop Mode. The font inflation checks happen at [1] and so we would need to probably add a function to the DOMWindowUtils or something to get a the presshell and set a flag on it while desktop mode is enabled. That flag would then be read in the function at [1] to disable font inflation.

If you're fine with working in C++ then I'm happy to guide you through this. Let me know!

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=ab1bcaf07693#11062
Sure, i am fully comfortable with c and c++ too and willing to work on this bug.
Great! I'm assigning the bug to you. I'm assuming you have a working build of Fennec since you've contributed patches to Fennec before.

I was thinking about this a little bit more and I realized there's probably a more correct solution than what I described in the last comment. The better approach would be to check if the tab is in document mode at [1] and then return the same default viewport that is returned when that pref is false. That will take care of the font inflation code as well as anything else that might be relying on the meta-viewport properties.

So what we'll want to do is add a flag mDesktopModeViewport to the nsPIDOMWindow class at [2]. We'll also add getter/setter functions for this flag on nsPIDOMWindow. Then we'll need to add a similar SetDesktopModeViewport(bool) function to the DOMWindowUtils [3] and implement it in [4] so that it calls the corresponding method on the nsPIDOMWindow. Then we need to update the code at [1] to read the flag from the window and return the default viewport if desktop mode is set. Finally, we'll need to modify the browser.js code in Fennec so that when desktop mode is enabled/disabled [5], we call the DOMWindowUtils function for that tab.

Take a look through the bits of code and let me know if that makes sense or not.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=af30b02139ef#7818
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h?rev=89858cf28204#69
[3] http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl?rev=d571e279fac4#55
[4] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp
[5] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=a275c8158e78#3504
Assignee: nobody → san13692
Thanx Kartikaya for detailed explanation ,i am on my way back to college and will start working on very soon.
Hi Surabhi, I just wanted to check in to see how you're doing. If you haven't gotten a chance to work on this yet that's fine, there's no rush - but if you do start working on it and get stuck please let me know so I can help you out. Thanks!
Hello, and i am sorry for delay but this bug really bugged me :D because the things to be done seemed so confusing but i finally tried and spend greater time to understand and here's what i understood along with queries(also i have nearly no experiance in website designing)-

> The
> better approach would be to check if the tab is in document mode at [1] and
> then return the same default viewport that is returned when that pref is
> false. That will take care of the font inflation code as well as anything
> else that might be relying on the meta-viewport properties.
> 

What actually Document mode is here ,apart from what i have understood based on searching that its basically how a page is rendered.?And what we want here is to set the viewport to full desktop view?

> So what we'll want to do is add a flag mDesktopModeViewport to the
> nsPIDOMWindow class at [2]. We'll also add getter/setter functions for this
> flag on nsPIDOMWindow.

So We want to add a flag here to get the status of which mode the tab is?What actually will that flag be ,a boolean to specify mode if its a desktop mode?
 
>Then we'll need to add a similar
> SetDesktopModeViewport(bool) function to the DOMWindowUtils [3] and
> implement it in [4] so that it calls the corresponding method on the
> nsPIDOMWindow. 

Does that means we have to create a funtion which will be called to set the mode of tab to desktop view.

>Then we need to update the code at [1] to read the flag from
> the window and return the default viewport if desktop mode is set. Finally,
> we'll need to modify the browser.js code in Fennec so that when desktop mode
> is enabled/disabled [5], we call the DOMWindowUtils function for that tab.
> 

So now we have to read flag to finally set desktop mode?

I am thankful and sorry if i am making it difficult for u to make me understand but i really want to try solving this.And these are confusions which i have above. I would also be greatful if u can give links to some implementations of things needed to be done above{if there exists any or similar to whats needed }
(In reply to surabhi anand from comment #8)
> Hello, and i am sorry for delay but this bug really bugged me :D because the
> things to be done seemed so confusing but i finally tried and spend greater
> time to understand and here's what i understood along with queries(also i
> have nearly no experiance in website designing)-

Not a problem at all! And it's not just you - this code is confusing to begin with and even people who have experience with it often have trouble. Let's see if we can work through it...

> > The
> > better approach would be to check if the tab is in document mode at [1] and
> > then return the same default viewport that is returned when that pref is
> > false. That will take care of the font inflation code as well as anything
> > else that might be relying on the meta-viewport properties.
> > 
> 
> What actually Document mode is here ,apart from what i have understood based
> on searching that its basically how a page is rendered.?And what we want
> here is to set the viewport to full desktop view?

Whoops, I meant "desktop mode" instead of "document mode" - my mistake! I can see how that would have been confusing, sorry. But yes you are right that the viewport basically affects how a page is rendered, and what we want to do here is when a tab is in "desktop mode" we want to use the full desktop viewport instead of what is specified by the meta-viewport tag. For some background reading on the meta-viewport tag you can see the page at https://developer.mozilla.org/en-US/docs/Mozilla/Mobile/Viewport_meta_tag

> > So what we'll want to do is add a flag mDesktopModeViewport to the
> > nsPIDOMWindow class at [2]. We'll also add getter/setter functions for this
> > flag on nsPIDOMWindow.
> 
> So We want to add a flag here to get the status of which mode the tab
> is?What actually will that flag be ,a boolean to specify mode if its a
> desktop mode?

Yes, exactly. In the browser.js file there is an object that represents a "tab", and that object knows if the tab is being rendered in desktop mode or not. Inside the gecko C++ code there is an nsPIDOMWindow for each tab, but that currently has no knowledge of desktop mode. So what we are trying to do here is to add a boolean in nsPIDOMWindow that basically reflects the knowledge from the tab in browser.js, so that in the C++ code we can know if the tab is in desktop mode or not.

> >Then we'll need to add a similar
> > SetDesktopModeViewport(bool) function to the DOMWindowUtils [3] and
> > implement it in [4] so that it calls the corresponding method on the
> > nsPIDOMWindow. 
> 
> Does that means we have to create a funtion which will be called to set the
> mode of tab to desktop view.

Yes. So in the paragraph above I said we add a boolean, and in this part what we are doing is updating that boolean so that it properly reflects whether the tab is in desktop mode.

> >Then we need to update the code at [1] to read the flag from
> > the window and return the default viewport if desktop mode is set. Finally,
> > we'll need to modify the browser.js code in Fennec so that when desktop mode
> > is enabled/disabled [5], we call the DOMWindowUtils function for that tab.
> > 
> 
> So now we have to read flag to finally set desktop mode?

Correct again. Once we have the nsPIDOMWindow correctly track whether or not it is in desktop mode, we read the flag and use that to decide which viewport we use. You referred to this as "set desktop mode" in your question but really it's more like "make desktop mode have the desired effect". Does that make sense?

> I am thankful and sorry if i am making it difficult for u to make me
> understand but i really want to try solving this.And these are confusions
> which i have above. I would also be greatful if u can give links to some
> implementations of things needed to be done above{if there exists any or
> similar to whats needed }

Not at all - I'm happy that you're digging through the code and making sense of all this, please continue to do so. I hope I was able to provide some clarifications, please let me know if that was not the case or if there's still something which is confusing. In terms of links to example of what needs to be done, I can point you to some things, yes.

For example, the mDesktopModeViewport flag that I asked you add to nsPIDOMWindow will be very similar to the mIsActive flag that is already in that class; see [1] and [2]. For the changes needed to the DOMWindowUtils, you can look at for example the enterModalState function [3][4] which accesses the nsPIDOMWindow; the function you add to propagate the desktop mode flag will look similar to this but you'll pass in a bool argument as well. For places that we use a DOMWindowUtils already in browser.js see for example [6].

Hope that helps, and please let me know if you have more questions.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h?rev=89858cf28204#87
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h?rev=89858cf28204#827
[3] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp?rev=d99b6fd9ca56#2422
[4] http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl?rev=42272b7f8e48#1301
[6] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=a275c8158e78#3726
Hi Surabhi, any progress on this? Let me know if you get stuck, or would like to abandon this bug to work on something else. Thanks!
Flags: needinfo?(san13692)
Sorry Kartikaya,I cant work on this bug right now.Thank you:)
Flags: needinfo?(san13692)
Assignee: san13692 → nobody
Hi Kartikaya,

I'm working on this bug.
and, I fixed some sources(like comment #5) and test it.
But it was not work well.
It is same view all page when desktop mode.

I have a some question for this bug.

-default viewport mean?
Comment #5 said that nsDocument#GetViewportInfo() shuld return default viewport when desktopmode.
Does this default viewport mean returned viewport when dom.meta-viewport.enable is false?

My modified code is as follows.
------
  // Get requested Desktopmode
  nsPIDOMWindow* win = GetWindow();
  if ( win && win->IsDesktopModeViewport() )
  {
    return nsViewportInfo(aDisplaySize,
                          defaultScale,
                          /*allowZoom*/false,
                          /*allowDoubleTapZoom*/ true);
  }
------

best regard.
Flags: needinfo?(bugmail.mozilla)
(In reply to mantaroh from comment #12)
> -default viewport mean?
> Comment #5 said that nsDocument#GetViewportInfo() shuld return default
> viewport when desktopmode.
> Does this default viewport mean returned viewport when
> dom.meta-viewport.enable is false?

Yes, if I understand your question. The "default viewport" I was referring to is the viewport that is returned when the dom.meta-viewport.enable pref is false, which is what you pasted in your code snippet.

Can you post the whole patch you're working with? I'm not sure how you're testing it - the meta viewport returned from that code will only affect whether font inflation is enabled or disabled. On the latest nightlies (as of bug 1127441) font inflation is disabled by default anyway, so I would not expect to see any difference from this change unless you manually enable font inflation.

Note also that I posted a patch to this bug in comment 0, and you will need to include that change in your final patch in order to get everything working and for the page to actually get rendered differently. The reason for that is that we have two separate pieces of code that computes the viewport and we use one in most places but in Fennec we use both. It's confusing but that's how it is for now.
Flags: needinfo?(bugmail.mozilla)
Posted patch 1106905.patch (obsolete) — Splinter Review
Hi Kartikaya,

Thank you for reply!

I misunderstand comment #0.
I thought that this patch is sample.

I try patch to my source, and confirmed action.
It's pretty good.

>an you post the whole patch you're working with? 
of course.
attached my patch.
But, It's in progress.
So not remove a debug log.

BTW, How do I turn off font inflation?
I saw the bug 1127441, I think that font.size.inflation.minTwips should set 120 in order to enable it.
Is it OK?
Flags: needinfo?(bugmail.mozilla)
Great! So font inflation should be turned off by default, and if you go into the settings -> Display -> Text Size you can turn it on by setting the text size to "Large" or "Extra Large" for example. If you load this bug page in the browser there should be a clear difference between having text size at "Tiny" vs having text size at "extra large", and that difference will happen regardless of whether you are in desktop mode or not. What we want is that once your patch is applied, going into desktop mode will disable the inflation, so the page should appear as though the text size was at Tiny. Does that make sense?
Assignee: nobody → mantaroh
Flags: needinfo?(bugmail.mozilla)
Posted patch 1106905.patch (obsolete) — Splinter Review
Hi Kartikaya,

I'm sorry to late reply.
and, thank you for font-inlation setting information.
I confirmed font-inflation when change text size setting.

>What we want is that once your patch is applied, going into desktop mode will disable the inflation, so the page should appear as though the text size was at Tiny. 
I checked that displayed tiny font size when request desktop mode.
And, attached my improve patch.

Could you please be review it?
Attachment #8573000 - Attachment is obsolete: true
Attachment #8574483 - Flags: review?(bugmail.mozilla)
Posted patch 1106905.patch (obsolete) — Splinter Review
hi kats,

Sorry, I attached wrong files.
So re-attach patch files.
Attachment #8574483 - Attachment is obsolete: true
Attachment #8574483 - Flags: review?(bugmail.mozilla)
Attachment #8574597 - Flags: review?(bugmail.mozilla)
Comment on attachment 8574597 [details] [diff] [review]
1106905.patch

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

This looks pretty good! I just have a few minor comments below. Also it would be good to update the commit message to be a little bit more descriptive - for example, "Modify mobile desktop mode implementation to use a desktop viewport".

Please update the patch to address my review comments and re-flag me for review. We'll also need to get a DOM peer to sign off on the changes; I can find somebody to do that once you post the updated patch. Thanks!

::: dom/base/nsDocument.cpp
@@ +7896,5 @@
>    CSSToScreenScale defaultScale = layoutDeviceScale
>                                  * LayoutDeviceToScreenScale(1.0);
> +  // Get requested Desktopmode
> +  nsPIDOMWindow* win = GetWindow();
> +  if ( win && win->IsDesktopModeViewport() )

nit: remove spaces after "(" and before ")"

::: dom/base/nsGlobalWindow.cpp
@@ +585,5 @@
>    mInnerWindow(nullptr), mOuterWindow(aOuterWindow),
>    // Make sure no actual window ends up with mWindowID == 0
>    mWindowID(NextWindowID()), mHasNotifiedGlobalCreated(false),
> +  mMarkedCCGeneration(0), mSendAfterRemotePaint(false),
> +  mDesktopModeViewport(false)

You'll have to move the initializer for mDesktopModeViewport up so that it appears in the same order as in the .h file, or this will fail to build on some platforms. In this case it needs to go before mInnerWindow.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1277,5 @@
>  
>    /**
> +   * Request set internal desktopMode flag change.
> +   */
> +  void SetDesktopModeViewport(in boolean aDesktopModeViewport);

In general the convention for IDL files is to start with a lowercase letter, so "setDesktopModeViewport" instead of "SetDesktopModeViewport". Same for the IsDesktopModeViewport. Note that the C++ functions will still start with an uppercase letter because the IDL processing code takes care of that. JS will start with lowercase.

In this case since it's just a simple getter/setter you could also turn it into an attribute, like so:

attribute boolean desktopModeViewport;

It's a little simpler but I don't feel strongly about this either way. You can see the isFirstPaint flag in this file for an example of how it works.
Attachment #8574597 - Flags: review?(bugmail.mozilla) → feedback+
Posted patch 1106905.patch (obsolete) — Splinter Review
Hi kats,

Thank you very much to confirm patch!
Sorry I didn't read the mozilla coding style guide.
I just was read this guide.
https://wiki.mozilla.org/Mobile/Fennec/Android#Coding_Style
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

I improved my patch and attached it.
Could you check it?

BTW, I concerned test which viewport and font-inflation.
Should I add a test-case?
and if you can , Could you run Try-Server?
Attachment #8574597 - Attachment is obsolete: true
Attachment #8575087 - Flags: review?(bugmail.mozilla)
Comment on attachment 8575087 [details] [diff] [review]
1106905.patch

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

Oh, I forgot one thing in my last review - any changes to functions in nsIDOMWindowUtils.idl requires a uuid bump. So run "mach uuid" and take the first line of output (a bunch of hex with dashes) and replace the existing uuid in the file (on line 53).

Other than that this looks good to me! I did a try push to https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f3ba4278cc8 with the change I mention below (otherwise it will fail compilation). Also flagging ehsan for a sanity check on the DOM bits.

::: dom/base/nsGlobalWindow.cpp
@@ +582,5 @@
>    mIsModalContentWindow(false),
>    mIsActive(false), mIsBackground(false),
>    mAudioMuted(false), mAudioVolume(1.0),
> +  mInnerWindow(nullptr), mDesktopModeViewport(false),
> +  mOuterWindow(aOuterWindow),

The mDesktopModeViewport needs to go before mInnerWindow.
Attachment #8575087 - Flags: review?(ehsan)
Attachment #8575087 - Flags: review?(bugmail.mozilla)
Attachment #8575087 - Flags: review+
That try push didn't run properly for some reason. I filed bug 1141606 for it and did another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98f706f23810
I also tried out the build from the try push above locally and it seems to work beautifully! Nice work, Mantaroh!
Comment on attachment 8575087 [details] [diff] [review]
1106905.patch

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

r=me with the below fixed.

::: dom/base/nsDOMWindowUtils.cpp
@@ +2389,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsDOMWindowUtils::IsDesktopModeViewport(bool *retval)

This function can go away (see my comment in the IDL file.)

::: dom/base/nsPIDOMWindow.h
@@ +95,5 @@
>      return mIsActive;
>    }
>  
>    // Outer windows only.
> +  virtual void SetDesktopModeViewport(bool aDesktopModeViewport)

This doesn't need to be virtual.

@@ +100,5 @@
> +  {
> +    MOZ_ASSERT(IsOuterWindow());
> +    mDesktopModeViewport = aDesktopModeViewport;
> +  }
> +  bool IsDesktopModeViewport()

Nit: please make this const.

@@ +106,5 @@
> +    MOZ_ASSERT(IsOuterWindow());
> +    return mDesktopModeViewport;
> +  }
> +
> +  // Outer windows only.

No need to repeat the comment here.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1282,5 @@
> +
> +  /**
> +   * Get current internal desktopMode flag.
> +   */
> +  boolean isDesktopModeViewport();

I don't see anywhere that this function is used, so I think you can remove it and its implementation from nsDOMWindowUtils.cpp.  You should also rev the uuid of this interface as kats mentioned.
Attachment #8575087 - Flags: review?(ehsan) → review+
Posted patch 1106905.patch (obsolete) — Splinter Review
Hi kats,ehsan,

I'm sorry to late reply.

Thank you so much for good advice!

I improved patch and test it.
So, Could you please review this patch?
Attachment #8575087 - Attachment is obsolete: true
Attachment #8576434 - Flags: review?(bugmail.mozilla)
Posted patch 1106905.patchSplinter Review
I'm sorry,
I forgot to write comments and author information to apply the patch to the file .
Attachment #8576434 - Attachment is obsolete: true
Attachment #8576434 - Flags: review?(bugmail.mozilla)
Attachment #8576441 - Flags: review?(bugmail.mozilla)
Comment on attachment 8576441 [details] [diff] [review]
1106905.patch

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

Looks great, thanks! I'll test this locally once more and then land it into the tree.
Attachment #8576441 - Flags: review?(bugmail.mozilla) → review+
Landed on the inbound tree:

https://hg.mozilla.org/integration/mozilla-inbound/rev/730f2da2e733

Once inbound gets merged to mozilla-central (should be in less than a day) this bug will be marked fixed. Thanks Mantaroh!
https://hg.mozilla.org/mozilla-central/rev/730f2da2e733
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Depends on: 1155177
Duplicate of this bug: 788921
You need to log in before you can comment on or make changes to this bug.