1032790 - In-content preferences: Close button missing from the Exceptions sub-dialog on Mac OSX

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Camelia Badau [:cbadau], Desktop Test Engineering]

This bug was filled based on the discussion from bug 1021618 comment 34.

Reproducible on the latest Nightly (BuildID: 20140630030202)
Mozilla/5.0 (Machintos; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0

Steps to reproduce: 
1. Launch Firefox.
2. Open about:preferences. 
3. Open any Exceptions sub-dialog. 

Actual results: 
The Close button is missing from all the Exceptions sub-dialogs on Mac OSX. 

Expected results: The Close button should be added to the Exceptions sub-dialogs on Mac OSX?

Comment 1

[Richard Marti (:Paenglab)]

I don't know who from UX can answer so I'm needinfoing :Boriss.

Jennifer, what do you think, should the close button also be shown on the Exceptions in-content sub-dialogs? Best you are looking on Content/Pop-ups - Exceptions... as the other are not yet implemented as in-content. But you can use the other as a reference for the normal dialogs.

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Screenshot: in-content sub-panel in OSX currently in nightlies

(In reply to Richard Marti (:Paenglab) from comment #1)
> I don't know who from UX can answer so I'm needinfoing :Boriss.
> 
> Jennifer, what do you think, should the close button also be shown on the
> Exceptions in-content sub-dialogs? Best you are looking on Content/Pop-ups -
> Exceptions... as the other are not yet implemented as in-content. But you
> can use the other as a reference for the normal dialogs.

The close button on the dialogs in Content can be a model for other dialogs.  The kerning and leading and margins in these dialogs need some improvement, but the grey close x (attached) is fine to apply to all in-content sub-dialogs.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: dialogCloseButton.patch

I think Jennifer had only the close button on top right in view.

But I think the "Close" button on bottom left should be shown also on OS X as only this dialogs have only the ability to close the dialog by the [X] button. All other dialogs have either a "Close" or "Cancel" button.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8469982 [details] [diff] [review]
dialogCloseButton.patch

This is intended behaviour to match the platform and I think your change to permissions.xul will affect when it's opened outside of in-content preferences. I think UX should be consulted to understand if/how we want to diverge from platform styling and when we want to do that (only for in-content preferences or for other dialogs too).

The alternative is to reverse this fix and remove the buttons from other dialogs on OS X because instant-apply is on for OS X and it matches the OS better.

Comment 5

[Matthew N. [:MattN]]

See comment 4

Comment 6

[Richard Marti (:Paenglab)]

Comment on attachment 8469982 [details] [diff] [review]
dialogCloseButton.patch

Removing r? until Philipp's feedback.

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I'm not sure I fully grasp the specific problem, so here's a more general approach :)

- In-content dialogs shouldn't auto-apply (the dialogs are modals, and modals usually don't auto-apply)
- Every in-content dialog should have a »Cancel/Done« button combo (there might be better labels for these)
- Every in-content dialog should have a close button [x] that does the same as the cancel button
- These rules apply to all operating systems
- None of these rules influences dialogs that are not in-content (we should look at these separately)

Sevaan, do you think this makes sense?

Comment 8

[Matthew N. [:MattN]]

OK, if we don't want the subdialogs to instant-apply on any OS then that's the opposite of the direction we were headed with in-content preferences (as we were going to turn it on for all OSs instead of just OS X for in-content subdialogs). It sounds like in-content preferences which aren't modal (i.e. the main panes) should instant-apply but the subdialogs shown from it shouldn't? Dialogs outside of in-content preferences should continue to follow OS behaviour (i.e. instantApply on OS X only).

I think the proposal from comment 7 makes sense. I'll wait to see what Sevaan says.

Comment 9

[Richard Marti (:Paenglab)]

The Exceptions dialog are always a kind of instant-apply. They are adding/removing sites from lists and there is no rollback when you click the close button.

Comment 10

[Sevaan Franks [:sevaan]]

What :phsla says in Comment 7 makes sense to me, but I'm concerned about :Paenglab's point in Comment 9. A Cancel button implies no changes will be applied when the modal is closed and any lists edited within will be restored. Can this be the case?

Comment 11

[Richard Marti (:Paenglab)]

On the dialogs I change there is a "Close" not a "Cancel" button and also no meaning of restoring. On Linux and Windows they are already visible.

Comment 12

[Sevaan Franks [:sevaan]]

Hey :paenglab,

I don't have VMWare installed, so I can't do this...would you mind posted a screenshot of an Exception window for Mac and the same one for Windows just so I can see both? Sorry for the trouble and thanks!

Comment 13

[Richard Marti (:Paenglab)]

Attachment: Dialog on Windows

Comment 14

[Richard Marti (:Paenglab)]

Attachment: Dialog on OS X

Comment 15

[Richard Marti (:Paenglab)]

I add a needinfo for sevaan to not forget this bug.

Comment 16

[Sevaan Franks [:sevaan]]

I /did/ forget! Sorry, and thanks for the prompt!

Yes, the close button should be added to the dialog. All other OSs have it, so we should include it for the sake of consistency.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: dialogCloseButton.patch

Okay, asking again for review with updated to tip patch after sevaan's positive feedback.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8492176 [details] [diff] [review]
dialogCloseButton.patch

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

This looks good. Comment #7 mentions a lot of requirements, but not all of those need to be fixed in this bug.

Comment 19

[Richard Marti (:Paenglab)]

Try-build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0d624aae7d13

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/5513ebaf921a

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5513ebaf921a

Comment 22

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Mac OSX 10.9.5 using latest Nightly 37.0a1 (buildID: 20141214030205).