Closed Bug 1032790 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox :: Preferences, defect)

33 Branch
All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox37 --- verified

People

(Reporter: cbadau, Assigned: Paenglab)

References

Details

Attachments

(4 files, 1 obsolete file)

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?
Blocks: 1021618
Flags: firefox-backlog+
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.
Flags: needinfo?(jboriss)
(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.
Flags: needinfo?(jboriss)
Attached patch dialogCloseButton.patch (obsolete) — Splinter Review
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8469982 - Flags: review?(jaws)
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.
Attachment #8469982 - Flags: feedback-
Comment on attachment 8469982 [details] [diff] [review]
dialogCloseButton.patch

Removing r? until Philipp's feedback.
Attachment #8469982 - Flags: review?(jaws)
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?
Flags: needinfo?(philipp) → needinfo?(sfranks)
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.
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.
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?
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.
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!
Flags: needinfo?(sfranks)
Attached image Dialog on Windows
Attached image Dialog on OS X
I add a needinfo for sevaan to not forget this bug.
Flags: needinfo?(sfranks)
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.
Flags: needinfo?(sfranks)
Okay, asking again for review with updated to tip patch after sevaan's positive feedback.
Attachment #8469982 - Attachment is obsolete: true
Attachment #8492176 - Flags: review?(MattN+bmo)
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.
Attachment #8492176 - Flags: review?(MattN+bmo) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5513ebaf921a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Iteration: --- → 37.2
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: camelia.badau
Verified fixed on Mac OSX 10.9.5 using latest Nightly 37.0a1 (buildID: 20141214030205).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.