Work - Switch to using the tab-modal prompt implementation available in desktop Firefox

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

Tracking

Details

(Whiteboard: feature=work [preview])

Attachments

(3 attachments, 12 obsolete attachments)

2.95 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
13.56 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
69.96 KB, patch
TimAbraldes
: review+
Details | Diff | Splinter Review
The browser needs to be able to display and request information in a manner that blocks interaction with the page displayed on the current tab (AKA tab-modal dialogs).  Example situations include: javascript alerts, authentication dialogs.

The browser needs to be able to display and request information in a manner that blocks interaction with the whole browser (AKA window-modal dialogs).  Example situations include: the "sync setup" dialog, "stay on page" dialogs.

The current implementation of dialogs in metro Firefox lives in browser/metro/components/PromptService.js and in browser/metro/base/content/browser-ui.js (in the DialogUI object).  There are some issues with the current implementation that should be addressed:


* browser/metro/components/PromptService.js should be cleared of code that is not relevant to metro
    Currently the file has separate code paths depending on whether it is called in the content process or the main process, but in metro we have no content process.  Also, the file contains code that can optionally use a fallback service for creating dialogs, but this fallback service is not available in immersive/metro Firefox.

* Dialogs should never cause "unresponsive script warning"
    Currently, javascript "alert" dialogs (and potentially other dialogs) cause this warning to appear if you don't dismiss the dialog

* Tab-modal dialog background should not cover browser UI
    If you create a javascript "alert," the dark background appears over the browser UI in addition to the page content

* Tab-modal dialogs should only display on the tab they were created on
   If you create a javascript "alert" and then switch tabs, the alert persists across tabs

* Tab-modal dialogs should not display over window-modal dialogs
    It is currently possible to display js alerts (which should be
tab-modal) over the "stay on page" dialog (which should be window-modal)

* Closing the browser should work correctly even if a dialog is currently displayed
    Closing the browser while a dialog (tab-modal or window-modal) is displayed causes the process to hang and be terminated by Windows

* It should not be possible to interact with a page while a dialog (tab-modal or window-modal) is showing
    If you create a javascript "alert" while on Google's homepage, you can ctrl+L to get to the URL bar, then tab to the text box on the page.  If you start typing, the page will respond (you'll see Google's autocomplete page appear)

* It should not be possible to interact with the browser chrome while a window-modal dialog is displaying
    It is currently possible to switch tabs while the "stay on page" or "sync setup" dialogs are displaying.  For the "stay on page" dialog, the dialog continues to display.  For the "sync setup" dialog, the dialog goes away.  This is because the dialogs are created using two different mechanisms.


Asa: I'm requesting needinfo? from you because I'm not sure how you'll want to organize this.  I've created a "dialogs" story and I plan to file a Work item that is basically "revamp PromptService.js," but maybe you want to create an epic with multiple stories?  Either way, this story will need to be fleshed out.
Flags: needinfo?(asa)
Moving this to the Planning Backlog pending Asa's review.
Blocks: 841997
No longer blocks: 855905
Hi Tim, my recommendation is that there should be one story (Bug 866304) that has multiple work item dependencies.  You could have one work item for each of the bullet points you've outlined in the description.  This will help you get a sense for the true scope of the work and the point estimate.  What do you think?

Asa can tell us if he wants to relate the story to an existing Epic or create a new one.
Flags: needinfo?(tabraldes)
Based on input from :fryn and :yuan, it sounds like we might be able to do away with window-modal dialogs.

:fryn mentioned that the "stay on page" dialog would be tab-modal on desktop Firefox except for technical limitations.

:yuan mentioned that the sync popup will be part of the flyout.

I imagine that any future or existing dialogs that we want to make window-modal could be part of a flyout instead.  I'm removing "window-modal" from this bug description; this bug will now track work on tab-modal dialogs in Firefox metro/immersive.  If we decide at some later point that we absolutely need window-modal dialogs, we can file another bug to track that work.


Once we have a user story for this bug, we can file work items for the work that needs to be done to implement the desired features/functionality.  I don't think it makes sense to file bugs describing the current state of dialogs in Firefox metro/immersive, since a new/revamped implementation will likely fix multiple issues while simultaneously completing the features we're interested in (tab-modal dialogs).
Flags: needinfo?(tabraldes)
Summary: Story - Browser can display information to the user in tab-modal and window-modal dialogs → Story - Browser can display information to the user in tab-modal dialogs
Created attachment 744855 [details] [diff] [review]
Patch v1 (WIP)

This patch removes the custom prompts implementation and uses the toolkit implementation of tab-modal prompts.  This is a WIP patch; it adds a lot of debugging output, breaks some functionality, and has a lot of TODOs in the code.

What works:
* Auth prompts, alerts, "stay on page" (these are the only prompts I knew how to test)
* Prompts do not persist across tabs
* The user cannot focus page elements when a prompt is active
* Prompts display on top of one another correctly

What doesn't work:
* Since this patch uses toolkit's tab-modal prompt implementation, the prompts are not metro-styled; they look like desktop's tab-modal prompts
* This patch makes desktop use tab-modal prompts for things that it normally doesn't (authentication dialogs, etc)
* It is possible to focus the elements of a prompt that is behind the top-most displaying prompt (maybe this is desired?)

This patch demonstrates that we can take advantage of toolkit's tab-modal prompt implementation in Firefox immersive/metro.  It might be possible to apply some of this patch to the prompt implementation that already exists in metro, or to apply our metro styling to the tab-modal prompts enabled by this patch.

Updated

6 years ago
Summary: Story - Browser can display information to the user in tab-modal dialogs → Story - Browser should display information to the user in tab-modal dialogs

Updated

6 years ago
Depends on: 864247

Updated

6 years ago
Flags: needinfo?(asa)
Created attachment 753572 [details] [diff] [review]
Part 1 patch v1

Removed logging.  Also made it so that the patch no longer causes desktop Firefox to use tab-modal dialogs.
Attachment #744855 - Attachment is obsolete: true
Created attachment 753575 [details] [diff] [review]
Part 2 Patch v1

This patch contains the changes to toolkit required to make Windows 8 Style Firefox use tab-modal dialogs, and not also force desktop Firefox to do so
Hi Tim, confirming if this story is ready to move into development for Iteration #8?
Flags: needinfo?(tabraldes)
Priority: P4 → P3
Yes, this story is ready to move into development for iteration 8.  I've written up a list of dependent work items and I'll file those now.

I'm raising the priority of this story to P1. I believe that we cannot ship unless we have working prompts. Please readjust if you disagree.
Flags: needinfo?(tabraldes)
Priority: P3 → P1
Perfect.  Thanks for the update Tim.  I'll get the story moved over.

Updated

6 years ago
Blocks: 875024
No longer blocks: 841997

Updated

6 years ago
QA Contact: jbecerra
Duplicate of this bug: 801172
No longer depends on: 864247
It turns out there was already a story for metro prompts.  I'm converting this bug into a work item since it already has patches, and linking it to the existing story.
No longer blocks: 875024
Summary: Story - Browser should display information to the user in tab-modal dialogs → Work - Switch to using the tab-modal prompt implementation available in desktop Firefox
Whiteboard: feature=story c=Find_in_page_app_bar u=metro_firefox_user p=8 → feature=work

Updated

6 years ago
Blocks: 831978
Comment on attachment 753572 [details] [diff] [review]
Part 1 patch v1

Hey Matt; with the patches in this bug, tab-modal prompts are functional but they look ugly (until we apply some styling per bug 801887).

Aside from the ugliness, there are two known issues:
  Bug 864247 - "Stay on page" dialog should only be able to appear once
  Bug 876042 - The new tab UI should not hide auth prompts

I think the overall buggy-ness of our prompts will be reduced after landing the patches in this bug, even with the known issues. Also it would be nice to get some coverage from having other people using tab-modal prompts in everyday testing. Do you think we should land the patches in this bug and iterate, or try to knock out some of the known issues first?
Attachment #753572 - Flags: review?(mbrubeck)
Comment on attachment 753575 [details] [diff] [review]
Part 2 Patch v1

Dolske; I added a hack to nsPrompter that lets metro tell it to default to tab-modal prompts.

Is this landable? Or is there maybe a smarter way to accomplish the same thing?
Attachment #753575 - Flags: review?(dolske)
> Hey Matt; with the patches in this bug, tab-modal prompts are functional but
> they look ugly (until we apply some styling per bug 801887).

Oops, I meant bug 801187.
Comment on attachment 753572 [details] [diff] [review]
Part 1 patch v1

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

::: browser/metro/base/content/browser-ui.js
@@ -717,5 @@
> -
> -    // Check open dialogs
> -    let dialog = DialogUI.activeDialog;
> -    if (dialog) {
> -      dialog.close();

We should still allow the Esc key to close open flyouts (e.g. the Options panel or the Sync pairing dialogs).  Maybe we could keep just the bare minimum of DialogUI to support this?

::: browser/metro/base/content/browser.js
@@ +44,5 @@
>  
>    startup: function startup() {
>      var self = this;
>  
> +    prompter =

let prompter =

@@ +410,5 @@
>      return this._tabs;
>    },
>  
> +  getTabModalPromptBox: function(aBrowser) {
> +    const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

You can put this const at the top level of browser.js to avoid repeating it.

@@ +418,5 @@
> +    let self = this;
> +
> +    let promptBox = {
> +      appendPrompt : function(args, onCloseCallback) {
> +	let newPrompt = document.createElementNS(XUL_NS, "tabmodalprompt");

Whitespace nit: some real tab characters snuck in here; please use only spaces (because tabs are not handled consistently across editors).

::: browser/metro/base/content/sync.js
@@ -148,5 @@
>      document.getElementById("syncsetup-simple").hidden = false;
>      document.getElementById("syncsetup-waiting").hidden = true;
>      document.getElementById("syncsetup-fallback").hidden = true;
>  
> -    DialogUI.pushDialog(this);

I think you missed another pushDialog/popDialog pair lower down in this file.
Attachment #753572 - Flags: review?(mbrubeck) → feedback+

Updated

5 years ago
Depends on: 845468
Comment on attachment 753575 [details] [diff] [review]
Part 2 Patch v1

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

Eep, I thought this was bigger but I was looking at the wrong patch. Sorry.

I don't think we should "fix" nsPrompter this way. The core problem is that there are a number of prompts which simply can't be tab-modal, because the code invoking them has implicit assumptions that the window-modalness (or are they app-model? I forget) makes certain things impossible. The canonical example is the onbeforeunload dialog shown when leaving/closing a page... If it's tab-modal, you can try to close the tab again while it's already being shown and it's very-much not expecting that. In general, any callers can have inconsistent / non-reentrant state when they're expecting a window-modal prompt.

So, what to do.

In the long-run we can make more things tab-modal. Although I don't remember offhand if they are chrome (or addon?) dialogs that can't be associated with a web page. (Master password, maybe? Not that's isn't a shining example of good UI!) So maybe you still need a window-modal equivalent anyway, or maybe Metro just drops it and eats the compat cost.

In the short-term, this should probably just throw an exception when it's called in a non-window-modal fashion on Metro.

Note that even with your patch, nsPrompter will only try to use a tab-modal prompt when the caller provides a window for it to be modal to (ie, when domWin != null).
Attachment #753575 - Flags: review?(dolske) → review-
Created attachment 761269 [details] [diff] [review]
like this

Something like this (needs an isMetro() implementation, of course).

Updated

5 years ago
Blocks: 886563
No longer blocks: 831978

Updated

5 years ago
Blocks: 801187
Created attachment 781938 [details] [diff] [review]
base patch

This patch replaces metro's 'PromptService' (which implements nsIPromptService and nsIPrompt, among others) with an implementation that simply forwards all calls to toolkit's nsPrompter.js. Before forwarding each call, it writes the "allowTabModal" property on the object obtained from nsPrompter.js, causing the created prompt to be tab-modal.

It also adds functionality to browser.js for creating tab modal prompts. One of the changes made as part of adding this functionality is replacing the <browser> with a <stack> of <browser> objects.

If you apply only this patch, tab-modal prompts should work but they will look like desktop prompts (they won't span the whole width of the screen). Also, prompts will not be able to appear in front of the start UI.
Attachment #753572 - Attachment is obsolete: true
Attachment #753575 - Attachment is obsolete: true
Attachment #761269 - Attachment is obsolete: true
Attachment #781938 - Flags: review?(mbrubeck)
Created attachment 781942 [details] [diff] [review]
layout patch - fixes issue with multiple onbeforeunload prompts

If nsDocumentViewer::PermitUnload is called and another call to nsDocumentViewer::PermitUnload has not yet returned (meaning that the user has not yet selected an option from the 'stay on page' prompt), then simply allow the unload to happen.

I just wanted to show that there are ways that we can fix/workaround this issue. The patch should probably go in bug 864247 and we should probably get UX input as to what exactly should happen when the user tries to do something to a tab that already has an onbeforeunload prompt active. For example, we could easily return false instead of true from any subsequent PermitUnload calls.
Attachment #781942 - Flags: review?(dolske)
Created attachment 781945 [details] [diff] [review]
toolkit patch - makes prompts look better in metro

What: Makes the resizing code of tabprompts.xml choose different sizes for metro and desktop.

Why: On desktop we only ever want prompts to be 60% of the content width. On metro we want to create a "strip" ("stripe?") effect where the prompt appears as a band across the entire width of the browser.


What: Adds code to nsPrompter.js to fail loudly if we're unable to create a tab-modal prompt on metro.

Why: There is no mechanism for creating window-modal dialogs on metro, so if nsPrompter fails to create a tabmodal prompt, it has failed to create a prompt at all and we should notify the consumer of such. Also Dolske mentioned that this functionality might be useful.
Attachment #781945 - Flags: review?(dolske)
Created attachment 781946 [details] [diff] [review]
DialogUI dialogs patch - removes unused/obsolete code from DialogUI

Removes the "dialog" portions of DialogUI.

The "dialog" concept is unused (the last consumer was the sync setup UI which is now a flyout) and is obsolete since we have prompts.
Attachment #781946 - Flags: review?(mbrubeck)
Created attachment 781947 [details] [diff] [review]
DialogUI modals

Removes the "modals" portions of DialogUI. Converts the crash reporter prompt to use nsIPromptService.confirmEx instead of DialogUI.importModal.

The crash reporter prompt is the only consumer of DialogUI.importModal and related code. The "modals" concept that DialogUI implements should go away in favor of tab-modal dialogs and tab-agnostic notifications (e.g. flyouts). I don't think that the crash reporter prompt should be a tab modal dialog; I'd prefer to see it become an about: page or perhaps a flyout. Making it a tab-modal prompt was just the easiest thing to do in this set of patches.
Attachment #781947 - Flags: review?(mbrubeck)
Created attachment 781950 [details] [diff] [review]
866304startUI.patch

What: Makes the metro start page an XBL binding, and creates one of it per tab.

Why: The way we're inserting tab-modal prompts into the document tree made it impossible to have them appear in front of the start page (or alternatively, the way we're showing the start page made it impossible to display behind tab-modal prompts). I don't like these StartUI changes and I don't want to land them; there is other StartUI work going on that I think is preferable and I'd like to coordinate with (bug 851130). In the meantime, these changes make the patch easier to test and show that we have ways of working around the "prompts behind start screen" issue.
Okey dokey...

Attached are a bunch of patches that add tab-modal prompt support to Firefox metro. With these patches applied (really with just the base patch applied), all prompts created through the nsIPromptService and nsIPrompt interfaces should be created tab-modal.

As Dolske points out, sometimes there are consumers of nsIPromptService and nsIPrompt that don't handle re-entrancy very well. For onbeforeunload, I've included a simple patch that makes it so that, if the user does something that would trigger an onbeforeunload while an onbeforeunload prompt is already active, we just go ahead and allow the unload. This behavior seems intuitive to me when I use the browser, but someone in UX should probably sign off on it (or suggest better behavior). I'm not aware of other consumers that might have issues in metro (Master Password won't be an issue since we've dropped support for that in metro). If there are other problematic consumers, I'd like to take an approach of trying to fix them where possible.

Some use cases of nsIPromptService/nsIPrompt don't really make sense to attach to a specific tab. The examples I've come across are the remote debugging prompt ("do you want to allow this remote debugging connect?") and the crash reporter prompt ("do you want to enable crash reporting?"). For each of these cases, I think it would make sense to switch to some other notification mechanism like flyout panels or about: pages, or maybe even just open a blank tab for the prompt to be attached to. In the short-term, I don't think it's a terrible experience to have these notifications attached to a specific tab, but maybe others disagree.

One issue that occurs with these patches applied is that the prompt buttons don't accept touch input. Touch input was working until very recently, and I'm not sure whether a m-c change or a local change caused it to break. I'll track this down, but note that our current prompst implementation also experiences this issue (bug 840855).
I also just discovered that I broke auth prompts at some point. I'll track this down too :)
Comment on attachment 781950 [details] [diff] [review]
866304startUI.patch

I just did a clean build. It turns out that this patch is totally broken. On the plus side, touch input is working on the prompt buttons!
Attachment #781950 - Attachment is obsolete: true
Created attachment 782065 [details] [diff] [review]
base patch: apply this first!

Fixed auth prompts!
Attachment #781938 - Attachment is obsolete: true
Attachment #781938 - Flags: review?(mbrubeck)
Attachment #782065 - Flags: review?(mbrubeck)
Attachment #782065 - Attachment description: 866304.patch → base patch: apply this first!
Comment on attachment 782065 [details] [diff] [review]
base patch: apply this first!

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

Nice!

::: browser/metro/base/content/browser.js
@@ +970,5 @@
> +          if (json.permit) {
> +              let tab = this.getTabForBrowser(browser);
> +              BrowserUI.animateClosingTab(tab);
> +          }
> +          break;

Nit: It looks like the indentation on these lines changed for no good reason.

::: browser/metro/components/PromptService.js
@@ +1,1 @@
> +// -*- Mode: js2; tab-width: 2; indent-tabs-mode: nil; js2-basic-offset: 2; js2-skip-preprocessor-directives: t; -*-

Want to add a "use strict"; while you're up here? ;)

@@ +39,5 @@
>  
> +  callProxy: function(aName, aArgs) {
> +    let domWin = aArgs[0];
> +    if (!domWin) {
> +      domWin = Services.wm.getMostRecentWindow("navigator:browser");

nit: I don't like how we store the chrome window in this variable temporarily, just to overwrite it with a content window.  This code would be clearer to me if we used a separate variable for the chrome window (i.e. replace the line above with "let chromeWin = ...").
Attachment #782065 - Flags: review?(mbrubeck) → review+
Attachment #781947 - Flags: review?(mbrubeck) → review+
Attachment #781946 - Flags: review?(mbrubeck) → review+
Comment on attachment 781942 [details] [diff] [review]
layout patch - fixes issue with multiple onbeforeunload prompts

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

I can't review this piece, you'll need a layout peer for that.

One concern, though, is checking to see if the current code here is OK with being reentrant, and what (if any) bad things might happen when the 2nd call terminates the page. Especially because PermitUnload() is an exposed API, so this concern extends to other things (like frontend code) that might be blocked in one event loop waiting for a response, and having another event loop kill it off leaving things in a weird state. :(

The doc unloading stuff may need a fair bit of refactoring to make this all async, which would avoid this whole problem.

A safer workaround would be to simply require that the existing prompt be used...

  if (mInPermitUnloadPrompt) {
    *aPermitUnload = false;
    return NS_OK;
  }
  ...
  mInPermitUnloadPrompt = true;
  rv = prompt->ConfirmEx(...)
  mInPermitUnloadPrompt = false;

Oh, and you'd want this check further up to prevent firing multiple |beforeunload| events.
Attachment #781942 - Flags: review?(dolske)
Comment on attachment 781945 [details] [diff] [review]
toolkit patch - makes prompts look better in metro

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

::: toolkit/components/prompts/content/tabprompts.xml
@@ +167,5 @@
> +                    let metroUtils = Cc["@mozilla.org/windows-metroutils;1"]
> +                                     .createInstance(Ci.nsIWinMetroUtils);
> +                    this.isMetro = metroUtils && metroUtils.immersive;
> +                } catch (e) {
> +                }

Does .immersive just indicate if you're running in, uhm, what most people call Metro vs the classic Win8 desktop?

.getService() instead of .createInstance?

I wonder if we should just add this interface to Services.jsm as |Services.metro|.

Nit: I've generally preferred using |"nsIFoo" in Ci| to avoid the try/catch + exception, especially since it hides a real exception being thrown when it shouldn't happen. Assuming this interface doesn't exist on non-Windows.

Ends up nice as:

  if (Services.metro && Services.metro.immersive) { ... } 

;-)

@@ +181,1 @@
>                    this.ui.infoTitle.removeAttribute("hidden");

I think this should actually just be a check for a chrome principal, instead of Metro. (It just happens that only Metro uses it this way right now.) That would also avoid making every content prompt say "The page at http://site.com says:" at the top.

Though I suppose that touches on another point -- will the Metro prompt styles differ for content vs chrome? Maybe .infoTitle _should_ always be shown for content prompts on Metro, to help prevent spoofing.

FWIW, Desktop's tab-modal prompts were explicitly styled to be neutral / non-native, to help a bit with that cue.

@@ +251,5 @@
> +                let maxHeight;
> +                if (this.isMetro) {
> +                    maxWidth = availWidth + info.clientWidth - main.clientWidth;
> +                    this.minWidth = maxWidth;
> +                    body.style.minWidth = this.minWidth + "px";

Gah. This (existing) code is so confusing, I just don't remember how all the magic works. :(

If you're getting acceptable results with the test cases in http://dolske.net/mozilla/tests/prompt/sizes.html, it's probably ok.

But does the existing (unpatched) code happen to magically work if you just tweak toolkit/components/prompts/content/tabprompts.xml to be:

  .mainContainer {
    min-width: 100%;
    min-height: 12em;
  }

@@ +255,5 @@
> +                    body.style.minWidth = this.minWidth + "px";
> +                } else {
> +                    maxWidth = Math.max(Math.floor(availWidth * 0.6),
> +                                        this.minWidth)
> +                             + info.clientWidth - main.clientWidth;

This is really nonstandard formatting. Please keep the existing style.

  maxWidth = Math.max(x,y) +
             info.clientWidth - main.clientWidth;

@@ +262,5 @@
> +                }
> +
> +                maxHeight = Math.max(Math.floor(availHeight * 0.6),
> +                                     this.minHeight)
> +                          + info.clientHeight - main.clientHeight;

Ditto.

::: toolkit/components/prompts/src/nsPrompter.js
@@ +878,5 @@
> +  } catch (e) {
> +  }
> +
> +  return false;
> +});

This would be another good reason for adding this API to Services.jsm. :)
Attachment #781945 - Flags: review?(dolske) → review-
Comment on attachment 781942 [details] [diff] [review]
layout patch - fixes issue with multiple onbeforeunload prompts

(In reply to Justin Dolske [:Dolske] from comment #29)
> Comment on attachment 781942 [details] [diff] [review]
> layout patch - fixes issue with multiple onbeforeunload prompts
> 
> Review of attachment 781942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't review this piece, you'll need a layout peer for that.

Sending review to roc.

roc: Please delegate as necessary.

> One concern, though, is checking to see if the current code here is OK with
> being reentrant, and what (if any) bad things might happen when the 2nd call
> terminates the page. Especially because PermitUnload() is an exposed API, so
> this concern extends to other things (like frontend code) that might be
> blocked in one event loop waiting for a response, and having another event
> loop kill it off leaving things in a weird state. :(

This seemed to work fine in my testing (closing a tab worked properly even if a beforeunload prompt was active when I tried to close the tab). I didn't run through in the debugger to verify that all internal state is consistent, but there are comments/code nearby that make me believe that this potential issue has already been dealt with [1].

[1] https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?rev=66f4834afb70#1124

> The doc unloading stuff may need a fair bit of refactoring to make this all
> async, which would avoid this whole problem.
> 
> A safer workaround would be to simply require that the existing prompt be
> used...
> 
>   if (mInPermitUnloadPrompt) {
>     *aPermitUnload = false;
>     return NS_OK;
>   }
>   ...
>   mInPermitUnloadPrompt = true;
>   rv = prompt->ConfirmEx(...)
>   mInPermitUnloadPrompt = false;

I'm cool with returning (through the out param) false instead of returning true, as mentioned in comment 19.

> Oh, and you'd want this check further up to prevent firing multiple
> |beforeunload| events.

In the patch, we check this at the beginning of the PermitUnload function, well before the |beforeunload| event is fired. It sounds like we would want to pull the |mInPermitUnload| check into its own |if| block.
Attachment #781942 - Flags: review?(roc)
Created attachment 783617 [details] [diff] [review]
866304toolkit.patch

(In reply to Justin Dolske [:Dolske] from comment #30)
> Comment on attachment 781945 [details] [diff] [review]
> toolkit patch - makes prompts look better in metro
> 
> Review of attachment 781945 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/prompts/content/tabprompts.xml
> @@ +167,5 @@
> > +                    let metroUtils = Cc["@mozilla.org/windows-metroutils;1"]
> > +                                     .createInstance(Ci.nsIWinMetroUtils);
> > +                    this.isMetro = metroUtils && metroUtils.immersive;
> > +                } catch (e) {
> > +                }
> 
> Does .immersive just indicate if you're running in, uhm, what most people
> call Metro vs the classic Win8 desktop?

Yup! That's exactly what .immersive indicates

> .getService() instead of .createInstance?

I'm not sure about the specifics here; I just know that the example I found [1] uses .createInstance

If there's some kind of conversion/refactoring I could perform to make nsIWinMetroUtils a getService-able service, I'd be happy to do that as a follow-up!

[1] https://mxr.mozilla.org/mozilla-central/source/browser/base/content/sync/setup.js?rev=c75172789a3c#424

> I wonder if we should just add this interface to Services.jsm as
> |Services.metro|.

I like this idea. I do feel it's a little out of scope for this bug though; mind if I do this as part of the follow-up work?

> Nit: I've generally preferred using |"nsIFoo" in Ci| to avoid the try/catch
> + exception, especially since it hides a real exception being thrown when it
> shouldn't happen. Assuming this interface doesn't exist on non-Windows.

Neat! I was unaware of that idiom, but I like it. Fixed in the attached patch.

> Ends up nice as:
> 
>   if (Services.metro && Services.metro.immersive) { ... } 
> 
> ;-)

We'll get there... :)

> @@ +181,1 @@
> >                    this.ui.infoTitle.removeAttribute("hidden");
> 
> I think this should actually just be a check for a chrome principal, instead
> of Metro. (It just happens that only Metro uses it this way right now.) That
> would also avoid making every content prompt say "The page at
> http://site.com says:" at the top.

Yesssss, thank you. I added that |this.isMetro| condition when I realized that none of the chrome prompts were displaying their titles; revealing the content prompt titles was an annoying side effect.

For this updated patch I added a call to |isSystemPrincipal| but I'm never seeing that return true. I'll investigate more in the morning but is this pretty much what you meant I should do?

> Though I suppose that touches on another point -- will the Metro prompt
> styles differ for content vs chrome? Maybe .infoTitle _should_ always be
> shown for content prompts on Metro, to help prevent spoofing.
> 
> FWIW, Desktop's tab-modal prompts were explicitly styled to be neutral /
> non-native, to help a bit with that cue.

I don't think there's anything we can do to the prompt itself to make it un-spoofable. If we have chrome prompts show their titles, then websites will just add "The page at http://site.com says:" at the top of their fake prompts. Preventing spoofing, if we're serious about it, should be a separate, longer discussion. My initial thought is that we'd have to have chrome prompts interact with the junior-style-overlay buttons since those are the only visible piece of chrome UI during regular browsing and since content can't interact with them.

> @@ +251,5 @@
> > +                let maxHeight;
> > +                if (this.isMetro) {
> > +                    maxWidth = availWidth + info.clientWidth - main.clientWidth;
> > +                    this.minWidth = maxWidth;
> > +                    body.style.minWidth = this.minWidth + "px";
> 
> Gah. This (existing) code is so confusing, I just don't remember how all the
> magic works. :(
> 
> If you're getting acceptable results with the test cases in
> http://dolske.net/mozilla/tests/prompt/sizes.html, it's probably ok.

Yup, that was the main page I used for testing.

> But does the existing (unpatched) code happen to magically work if you just
> tweak toolkit/components/prompts/content/tabprompts.xml to be:
> 
>   .mainContainer {
>     min-width: 100%;
>     min-height: 12em;
>   }

I spent waaaaaaaaay too long trying to get the code to magically work by adding percentages to the CSS. When I asked in #fx-team, I learned that "Percentages don't tend to work well in XUL." But why am I telling you this? You were there! [2]

[2] http://logbot.glob.com.au/?c=mozilla%23fx-team&s=17+Jul+2013&e=17+Jul+2013&#c27718

> @@ +255,5 @@
> > +                    body.style.minWidth = this.minWidth + "px";
> > +                } else {
> > +                    maxWidth = Math.max(Math.floor(availWidth * 0.6),
> > +                                        this.minWidth)
> > +                             + info.clientWidth - main.clientWidth;
> 
> This is really nonstandard formatting. Please keep the existing style.
> 
>   maxWidth = Math.max(x,y) +
>              info.clientWidth - main.clientWidth;
> 

Fixed.

> @@ +262,5 @@
> > +                }
> > +
> > +                maxHeight = Math.max(Math.floor(availHeight * 0.6),
> > +                                     this.minHeight)
> > +                          + info.clientHeight - main.clientHeight;
> 
> Ditto.

Fixed.

> ::: toolkit/components/prompts/src/nsPrompter.js
> @@ +878,5 @@
> > +  } catch (e) {
> > +  }
> > +
> > +  return false;
> > +});
> 
> This would be another good reason for adding this API to Services.jsm. :)

Heh, I agree!
Attachment #781945 - Attachment is obsolete: true
Attachment #783617 - Flags: review?(dolske)
Created attachment 783958 [details] [diff] [review]
base patch v2

(In reply to Matt Brubeck (:mbrubeck) from comment #28)
> Comment on attachment 782065 [details] [diff] [review]
> base patch: apply this first!
> 
> Review of attachment 782065 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!

:)

> ::: browser/metro/base/content/browser.js
> @@ +970,5 @@
> > +          if (json.permit) {
> > +              let tab = this.getTabForBrowser(browser);
> > +              BrowserUI.animateClosingTab(tab);
> > +          }
> > +          break;
> 
> Nit: It looks like the indentation on these lines changed for no good reason.

Reverted. The spacing changed because those are *shudder* real tab characters. Some day maybe I'll come back to this file and, with great restraint, make a whitespace-only change.

> ::: browser/metro/components/PromptService.js
> @@ +1,1 @@
> > +// -*- Mode: js2; tab-width: 2; indent-tabs-mode: nil; js2-basic-offset: 2; js2-skip-preprocessor-directives: t; -*-
> 
> Want to add a "use strict"; while you're up here? ;)

I got bit by adding "use strict"; as part of a bigger patch in bug 845468. I'll hold off on this for now, but I won't lose sleep over it because we have bug 898125 :)

> @@ +39,5 @@
> >  
> > +  callProxy: function(aName, aArgs) {
> > +    let domWin = aArgs[0];
> > +    if (!domWin) {
> > +      domWin = Services.wm.getMostRecentWindow("navigator:browser");
> 
> nit: I don't like how we store the chrome window in this variable
> temporarily, just to overwrite it with a content window.  This code would be
> clearer to me if we used a separate variable for the chrome window (i.e.
> replace the line above with "let chromeWin = ...").

Fixed!


NOTE: I can't actually test this updated patch locally yet (bug 89948) but I'll of course build and test. Carrying forward r+ in the meantime.
Attachment #782065 - Attachment is obsolete: true
Attachment #783958 - Flags: review+
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #33)
[...]
> NOTE: I can't actually test this updated patch locally yet (bug 89948) but
> I'll of course build and test. Carrying forward r+ in the meantime.

I of course meant bug 899948, not bug 89948.

Updated

5 years ago
Blocks: 900038
Whiteboard: feature=work → feature=work [preview]
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #32)

> > I wonder if we should just add this interface to Services.jsm as
> > |Services.metro|.
> 
> I like this idea. I do feel it's a little out of scope for this bug though;
> mind if I do this as part of the follow-up work?

Fair, but it's a 1-liner. :) Do it in a separate bug, and I'll review it lightning quick!

> > Though I suppose that touches on another point -- will the Metro prompt
> > styles differ for content vs chrome? Maybe .infoTitle _should_ always be
> > shown for content prompts on Metro, to help prevent spoofing.
> 
> I don't think there's anything we can do to the prompt itself to make it
> un-spoofable.

If there's no way for a user to tell visually or by other obvious means, then you should always show the title for content prompts. We're able to suppress the title on Desktop only because it's plainly obvious that the prompt is part of the page.

But I'm still not quite sure what this patch actually looks like, so the more significant problem would be how a user can tell when it's a chrome prompt (or more generally, trusted UI) that they're interacting with.


> But why am I telling you this? You were there! [2]

*blush*
Comment on attachment 781942 [details] [diff] [review]
layout patch - fixes issue with multiple onbeforeunload prompts

I'm moving this patch over to bug 864247, where it actually belongs.
Attachment #781942 - Attachment is obsolete: true
Attachment #781942 - Flags: review?(roc)
No longer blocks: 801187
No longer blocks: 900038
Comment on attachment 783617 [details] [diff] [review]
866304toolkit.patch

To reduce cognitive burden, I landed the 3 reviewed patches on fx-team:
    - base - https://hg.mozilla.org/integration/fx-team/rev/516063917157
    - DialogUI dialogs - https://hg.mozilla.org/integration/fx-team/rev/e0061be20d1e
    - DialogUI modals - https://hg.mozilla.org/integration/fx-team/rev/14434d2b75b9

And I'll move the remaining toolkit patch over to bug 801187, since that's a more appropriate place to discuss/work on the styling of our metro prompts.
Attachment #783617 - Attachment is obsolete: true
Attachment #783617 - Flags: review?(dolske)
seems this caused a test failure https://tbpl.mozilla.org/php/getParsedLog.php?id=26203038&tree=Fx-Team in the metro mochitests, retrigger didn't helped so i guess its not a Intermittent failure and i had to backout this change
Hmm, when I ran the tests locally they all passed. I'll see if I can get this reproducing locally.

It looks like the test is trying to read one of the localized strings that were removed.
Blocks: 853730
Created attachment 786628 [details] [diff] [review]
DialogUI modals removal

So it turns out that I had removed a line from browser/metro/locales/jar.mn incorrectly while rebasing.  This meant that strings used by the crash reporter prompt were unavailable. The browser_crashprompt.js mochitest was trying to make the crash reporter prompt appear, but was running into the missing string issue.

The reason this didn't come up in my local testing is that browser_crashprompt.js doesn't run at all if it determines that the crash reporter is disabled, which apparently it always is in my local builds.

I've updated the "DialogUI dialogs removal" patch to include a revamp of the browser_crashprompt.js mochitest.

I've verified (again) that all tests run locally, but I'll go ahead and do a try run with these 3 patches applied, just to be extra sure.

Matt: Nothing substantive has changed in the patch except for the test logic; could you do a once-over of just browser_crashprompt.js?
Attachment #781947 - Attachment is obsolete: true
Attachment #786628 - Flags: review?(mbrubeck)
Created attachment 786629 [details] [diff] [review]
866304 base patch v3

This is a correctly rebased version of the base patch. Carrying forward r+
Attachment #783958 - Attachment is obsolete: true
Attachment #786629 - Flags: review+
Attachment #786628 - Flags: review?(mbrubeck) → review+
Duplicate of this bug: 902254
Tryserver run looks good, let's try this again:
  - Base patch - https://hg.mozilla.org/integration/fx-team/rev/5becd6830a7d
  - DialogUI dialogs cleanup - https://hg.mozilla.org/integration/fx-team/rev/4716cc685a06
  - DialogUI modals cleanup (includes converting crashprompt to tabmodal dialog and updated browser_crashprompt.js test) - https://hg.mozilla.org/integration/fx-team/rev/daf88d34960c
Blocks: 893735
Blocks: 900038
Depends on: 949333
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.