394682 - Site-specific zoom applies to error pages

Status: NEW

(Firefox :: Tabbed Browser, defect, P3)

Description

[Phil Ringnalda (:philor)]

Wanting to have a site's text zoomed doesn't really translate to wanting to have XUL error pages from it, or even its feed in Firefox's preview, zoomed.

STR:
1. Visit http://daringfireball.net/ and press ctrl-+ until it's readable, two or three notches up ought to do it.
2. File menu, "Work Offline"
3. Click one of the star icons at the end of a title (the permalinks for blog posts), or any other interior link.
4. Note that the error page is startlingly large.
5. Work Online again, go back to daringfireball.net, and click the feed icon in the addressbar
6. Note that not only is the text for items too large, the subscription UI (which will never be altered by the site, unlike the item text, which may or may not) is, too.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Hmm, indeed, we should only be applying the zoom value to content pages and not apply it to chrome pages we happen to display in the content window (like error pages and the feed preview).

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Hmm, this may be tricky.  Feeds and error pages both use a similar technique to load chrome into the content window, which is to define an nsIAboutModule component that redirects the load to an about: URL that loads a chrome page.

Error pages use nsAboutRedirector to redirect to about:neterror, which loads chrome://global/content/netError.xhtml, while feeds use nsAboutFeeds to redirect to about:feeds, which loads chrome://browser/content/feeds/subscribe.xhtml.

These redirects can be detected from chrome by watching pageshow, but that seems likely to be an expensive way of doing it, given how often that event fires.  I'm not yet sure what a better approach would be, though.  Researching further.

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #2)
> Error pages use nsAboutRedirector to redirect to about:neterror, which loads
> chrome://global/content/netError.xhtml

You can detect about:neterror through gBrowser.contentDocument.documentURI.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> (In reply to comment #2)
> > Error pages use nsAboutRedirector to redirect to about:neterror, which loads
> > chrome://global/content/netError.xhtml
> 
> You can detect about:neterror through gBrowser.contentDocument.documentURI.

Right, and I think we can detect about:feeds the same way, but not on location change, since redirects to about: pages don't prompt location change events, as far as I can tell.  We'll have to listen for another event.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

With lots of help from biesi and twanno on IRC, I figured out how to handle these two cases.


For error pages, the following expression returns true onLocationChange for error page loads:

  (aRequest && !Components.isSuccessCode(aRequest.status))

However, error page loads also fire an extraneous second onLocationChange with aRequest == null, which causes that expression to return false, so we have to watch for and handle that second event.

One simple way to do so would be to check aWebProgress.isLoadingDocument, which will return true for regular page loads but false for error page loads, the second onLocationChange, and tab switches.

But currently we do have to apply page zoom on tab switches, so unless and until bug 386835 gets fixed in a way that no longer applies page zoom on tab switches, we'll have to instead check for aRequest == null.


Feed pages require a different solution.  onLocationChange fires after the browser has redirected to about:feeds, fortunately, but isLoadingDocument is true for feeds, and their request status doesn't reveal the redirection.

However, aRequest.nsIChannel.URI.spec points to the URL to which about:feeds resolves, i.e. the file: equivalent of chrome://browser/content/feeds/subscribe.xhtml as defined in nsAboutFeeds.cpp <http://lxr.mozilla.org/seamonkey/source/browser/components/feeds/src/nsAboutFeeds.cpp#45>, so we can look at the URI to determine whether or not the location change is to about:feeds.

Comment 7

[Shawn Wilsher :sdwilsh]

Myk - do you plan on getting a patch for this bug?

Comment 8

[Myk Melez [:myk] [@mykmelez]]

I had been planning to work on it, but then it slipped my mind.  I'm still interested in making it happen, although it won't be this week.  Updating the status to reflect that.  If you'd like to take it on, you are welcome to do so.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking for pre-approval, but I don't think we'd hold release for it.

Comment 11

[Mike Connor [:mconnor]]

Provisionally moving to P3, I don't think this blocks, but is instead wanted.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Yeah, would definitely take a patch; I thought there was one in the works (maybe that was another bug? they're all blurring together! oh noes!) but this won't block the release.

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P4 - Polish issue that is rarely encountered, and is easily identifiable.

Easy to notice when it is wrong, but requires the user to change the default text sizes, which limits the impact of the bug.

Comment 14

[Gurdit Singh Bedi]

[Tracking Requested - why for this release]: This bug will be affecting all the users. I am using firefox dev edition 57.0b64(64-bit) while reporting this issue.

Comment 15

[Marcia Knous [:marcia]]

Not tracking for Firefox 57.