This repository has been archived by the owner. It is now read-only.

FIX: restore b2g agent sheet scrollbars and other mobile tweaks on b2g windows #402

Merged
merged 4 commits into from Apr 3, 2013

Conversation

Projects
None yet
2 participants
@rpl
Member

rpl commented Mar 30, 2013

  • new DOM window detected by notification observers
  • inject full original b2g agent sheet css
    (e.g. to style buttons as in the FirefoxOS device)

should fix #379

NOTE: there're some remaining issues related to shell styling,
because currently any try to inject b2g agent sheet into the homescreen
iframe generates some weird side-effect (homescreen change rotation
instead to lock to portrait) but probably it's more important to restore the style for apps now, and then in a second time try to restore it on the homescreen as well.

@rpl

This comment has been minimized.

Show comment
Hide comment
@rpl

rpl Mar 30, 2013

Member

Screenshots of a couple of apps with reinjected b2g stylesheet:

app_with_mobile_scrollbar
firefoxos_browser_mobile_stylesheet

Member

rpl commented Mar 30, 2013

Screenshots of a couple of apps with reinjected b2g stylesheet:

app_with_mobile_scrollbar
firefoxos_browser_mobile_stylesheet

@rpl

This comment has been minimized.

Show comment
Hide comment
@rpl

rpl Mar 30, 2013

Member

Screenshots of a remaining issues on homescreen stylesheet:

style_issue_on_b2g_shell_style1
style_issue_on_b2g_shell_style2

Member

rpl commented Mar 30, 2013

Screenshots of a remaining issues on homescreen stylesheet:

style_issue_on_b2g_shell_style1
style_issue_on_b2g_shell_style2

@mykmelez

This comment has been minimized.

Show comment
Hide comment
@mykmelez

mykmelez Apr 1, 2013

Member

FloatingScrollbars.jsm looks like it is designed to enable a stylesheet to be added and removed on demand during the lifetime of a content frame. In this case, however, we only need to add a stylesheet once, when a content frame is first created; and we don't need to remove it.

So we should be able to do something simpler, like observing content window creation and adding a stylesheet to each content window that gets created. Here's a self-contained example that works in my tests, including for the homescreen:

Services.obs.addObserver(
  function injectStylesheet(subject, topic, data) {
    debug("injectStylesheet: " + data);
    let URL = Services.io.newURI("chrome://prosthesis/skin/b2g-content.css",
                                 null, null);
    let winUtils = subject.QueryInterface(Ci.nsIInterfaceRequestor).
                   getInterface(Ci.nsIDOMWindowUtils);
    winUtils.loadSheet(URL, winUtils.AGENT_SHEET);
  },
  "content-document-global-created",
  false
);

It should be straightforward to integrate that into SimulatorWindowManager, so we shouldn't need a separate B2GAgentSheet.jsm module.

Also, I think content-document-global-created will suit our purpose just fine, but note the others listed on Observer Notifications. If we run into problems adding the stylesheet to the right set of windows at the right time, then there are other options.

Member

mykmelez commented Apr 1, 2013

FloatingScrollbars.jsm looks like it is designed to enable a stylesheet to be added and removed on demand during the lifetime of a content frame. In this case, however, we only need to add a stylesheet once, when a content frame is first created; and we don't need to remove it.

So we should be able to do something simpler, like observing content window creation and adding a stylesheet to each content window that gets created. Here's a self-contained example that works in my tests, including for the homescreen:

Services.obs.addObserver(
  function injectStylesheet(subject, topic, data) {
    debug("injectStylesheet: " + data);
    let URL = Services.io.newURI("chrome://prosthesis/skin/b2g-content.css",
                                 null, null);
    let winUtils = subject.QueryInterface(Ci.nsIInterfaceRequestor).
                   getInterface(Ci.nsIDOMWindowUtils);
    winUtils.loadSheet(URL, winUtils.AGENT_SHEET);
  },
  "content-document-global-created",
  false
);

It should be straightforward to integrate that into SimulatorWindowManager, so we shouldn't need a separate B2GAgentSheet.jsm module.

Also, I think content-document-global-created will suit our purpose just fine, but note the others listed on Observer Notifications. If we run into problems adding the stylesheet to the right set of windows at the right time, then there are other options.

Show outdated Hide outdated prosthesis/skin/b2g-content.css Outdated
@rpl

This comment has been minimized.

Show comment
Hide comment
@rpl

rpl Apr 3, 2013

Member

So we should be able to do something simpler, like observing content window creation and adding a stylesheet to each content window that gets created.

@mykmelez awesome! it's much more simple and clean now.

I've just rebased this pull request on the current master and updated this pullrequest.

I wasn't aware of this notification, in my tests it's working as well as the previous prototype (using less code :-P),
it doesn't seem to be able to solve the remain corner cases.

In 4e18ea9 I've tweaked the suggested fix, injecting b2g.css stylesheet into the main FirefoxOS iframe
to fix the remain style issues (e.g. app switcher and mobile select)

Member

rpl commented Apr 3, 2013

So we should be able to do something simpler, like observing content window creation and adding a stylesheet to each content window that gets created.

@mykmelez awesome! it's much more simple and clean now.

I've just rebased this pull request on the current master and updated this pullrequest.

I wasn't aware of this notification, in my tests it's working as well as the previous prototype (using less code :-P),
it doesn't seem to be able to solve the remain corner cases.

In 4e18ea9 I've tweaked the suggested fix, injecting b2g.css stylesheet into the main FirefoxOS iframe
to fix the remain style issues (e.g. app switcher and mobile select)

@mykmelez

This comment has been minimized.

Show comment
Hide comment
@mykmelez

mykmelez Apr 3, 2013

Member

I wasn't aware of this notification, in my tests it's working as well as the previous prototype (using less code :-P),
it doesn't seem to be able to solve the remain corner cases.

Which corner cases remain? It looks like you've fixed the homescreen issues. Or are there still some issues with the homescreen?

Member

mykmelez commented Apr 3, 2013

I wasn't aware of this notification, in my tests it's working as well as the previous prototype (using less code :-P),
it doesn't seem to be able to solve the remain corner cases.

Which corner cases remain? It looks like you've fixed the homescreen issues. Or are there still some issues with the homescreen?

rpl added some commits Apr 3, 2013

prevents b2g stylsheet inject side-effect on running an app during b2…
…g startup

- move b2g stylesheet injection code into its own file (inject-b2g-agentsheet.js)
- observe "content-document-global-created" notification before ContentStart to
  inject into "iframe#homescreen" created inside shell.xul
  (prevents layouting bug on running app during b2g startup)
@rpl

This comment has been minimized.

Show comment
Hide comment
@rpl

rpl Apr 3, 2013

Member

Which corner cases remain? It looks like you've fixed the homescreen issues. Or are there still some issues with the homescreen?

@mykmelez my previous try (to fix the stylesheet issues in app switcher or mobile select) introduced a random
rendering/layouting bug when we run an app on a starting b2g instance, e.g. pressing the run button when
b2g desktop is not already running)

I fixed the bug and removed the side-effect in the last commit (91d85de):

  • move b2g stylesheet injection code into its own file (inject-b2g-agentsheet.js)
  • observe "content-document-global-created" notification before ContentStart to
    inject into "iframe#homescreen" created inside shell.xul
    (prevents layouting bug on running app during b2g startup)

now it should work correctly in all the cases (at least in the known cases)

Member

rpl commented Apr 3, 2013

Which corner cases remain? It looks like you've fixed the homescreen issues. Or are there still some issues with the homescreen?

@mykmelez my previous try (to fix the stylesheet issues in app switcher or mobile select) introduced a random
rendering/layouting bug when we run an app on a starting b2g instance, e.g. pressing the run button when
b2g desktop is not already running)

I fixed the bug and removed the side-effect in the last commit (91d85de):

  • move b2g stylesheet injection code into its own file (inject-b2g-agentsheet.js)
  • observe "content-document-global-created" notification before ContentStart to
    inject into "iframe#homescreen" created inside shell.xul
    (prevents layouting bug on running app during b2g startup)

now it should work correctly in all the cases (at least in the known cases)

@@ -0,0 +1,15 @@
const B2G_AGENTSHEET_URL = Services.io.newURI("chrome://prosthesis/content/b2g.css",

This comment has been minimized.

@mykmelez

mykmelez Apr 3, 2013

Member

This line exceeds 80 characters, so it would be better to format the statement as:

const B2G_AGENTSHEET_URL =
  Services.io.newURI("chrome://prosthesis/content/b2g.css", null, null);

Also, this file needs a license header.

@mykmelez

mykmelez Apr 3, 2013

Member

This line exceeds 80 characters, so it would be better to format the statement as:

const B2G_AGENTSHEET_URL =
  Services.io.newURI("chrome://prosthesis/content/b2g.css", null, null);

Also, this file needs a license header.

@mykmelez

This comment has been minimized.

Show comment
Hide comment
@mykmelez

mykmelez Apr 3, 2013

Member

Looks good! (modulo the minor issues in the previous comment)

Ideally, we would not create a separate JavaScript file for this small amount of code, but there isn't currently another file that could incorporate it sensibly. It would make sense to put it into shell.js, but we load that on ContentStart at the moment, because months ago we had trouble loading it early. But we should try that again, since the bug I encountered, which I didn't have time to investigate further back then, might well have been fixed by now.

Member

mykmelez commented Apr 3, 2013

Looks good! (modulo the minor issues in the previous comment)

Ideally, we would not create a separate JavaScript file for this small amount of code, but there isn't currently another file that could incorporate it sensibly. It would make sense to put it into shell.js, but we load that on ContentStart at the moment, because months ago we had trouble loading it early. But we should try that again, since the bug I encountered, which I didn't have time to investigate further back then, might well have been fixed by now.

@mykmelez

This comment has been minimized.

Show comment
Hide comment
@mykmelez

mykmelez Apr 3, 2013

Member

I'm going to fix the last two issues and then merge this so I can get it into the preview builds I'm about to spin.

Member

mykmelez commented Apr 3, 2013

I'm going to fix the last two issues and then merge this so I can get it into the preview builds I'm about to spin.

@mykmelez mykmelez merged commit 91d85de into mozilla:master Apr 3, 2013

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.