1304666 - Button order in Application Chooser is wrong on Windows

Status: RESOLVED DUPLICATE of bug 1580646

(Firefox :: File Handling, defect, P3)

Description

[Gian-Carlo Pascutto [:gcp]]

https://www.reddit.com/r/firefox/comments/53uqav/ff49_launch_application_dialog_buttons_in_reverse/

I verified this was an unintentional change from bug 510516.

Comment 1

[William Hsu [:whsu]]

Set priority to P2.

@ Drew, may I have a few minutes of your time to read this problem?
It seem to me it's a regression of bug 510516.
Thank you.

Comment 2

[Drew Willcoxon :adw]

It does seem like a regression from bug 510516.

Comment 3

[William Hsu [:whsu]]

(In reply to Drew Willcoxon :adw from comment #2)
> It does seem like a regression from bug 510516.

@ Drew, thanks for the information.
Are we going to fix this regression bug in this or next release?
(Not urgent. I just want to know the schedule.)

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Any updates here, Andrew?

Comment 5

[:Paolo Amadini]

Drew, would you like to mentor this?

Comment 6

[Drew Willcoxon :adw]

Sure.

Comment 8

[joeroberts1956]

Attachment: proposed patch

Proposed patch adding conditional causing "accept" and "cancel" button order to conform to Windows OS dialog standards while remaining compliant with UNIX and MAC OS standard.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

1) You need to mark a reviewer for your patch. You can ask [e-mail] for review, he's volunteered to do so in comment 6.

2) I think your patch is in a format Bugzilla doesn't understand. I think it prefers Unified diff format. See for example: https://wiki.mozilla.org/Bugzilla:Patches

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 10

[joeroberts1956]

Attachment: dialog.xul.v2.patch

I apologize for not understanding the mozreview commit/patch/review process...I barely understand the git/mercurial processes, the  last software I worked on used a check out and lock type process(check out prohibited any change by anyone until check in).  I also apologize in advance for errors I (surely will make) in my 2nd patch.  I thank all for their patience.  I would like feedback before a reviewer is requested.

Comment 11

[joeroberts1956]

Comment on attachment 8832806 [details] [diff] [review]
dialog.xul.v2.patch

Can I chat with the person who approved my patch?

I do not know how to implement testing for build targets other than my own hardware/OS

Comment 12

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

(Drive-by comment so it doesn't get lost in #introduction)
It would be good to create an actual commit that you export as a patch to attach to this bug. That will include a lot more patch metadata which is needed when it's actually time to get the patch landed.

General steps would be:
Modify files.
Make sure you've "hg add"ded all of your changed files.
| hg commit | (Give it a useful commit message that's in the format of "Bug [number] - [Description of patch]")
| hg export . > mypatch.patch | (This will export the patch to a file named mypatch.patch, located in your current directory)
Attach that patch file to the bug, marking the previous version(s) of the patch as obsolete.
Set the review or feedback flag to '?', and assign the review/feedback request to the reviewer (in this case, [e-mail]).

Once that's done, adw can review the patch, and can submit it to the tryserver for you to get builds and tests run against your patch.

Comment 13

[joeroberts1956]

Attachment: dialog.xul.v3.patch

Patch exported with description and proper formatting.  Not sure how to test.  A build for another target system would take 5+ hours on my computer.

Comment 14

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

Pushed this to tryserver, these builds should be done in a few hours to test with:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6008f15ec4c942c229772adabc45e5093ecb3298

Comment 15

[joeroberts1956]

Absolute failure.  Build system reconfigure was required before patch operates...

Comment 16

[joeroberts1956]

# HG changeset patch
# User Joe Roberts<[e-mail]>
# Date 1486700854 32400
#      Thu Feb 09 19:27:34 2017 -0900
# Node ID 13ab0a715f181aeb638f8575ce5ccc10cf70bfa5
# Parent  53c91ab46318b87396392568368fe1d6ecc8c082
Make dialog.xul a preprocessed file enforcing OS depenedent button orientation.

diff --git a/toolkit/mozapps/handling/jar.mn b/toolkit/mozapps/handling/jar.mn
--- a/toolkit/mozapps/handling/jar.mn
+++ b/toolkit/mozapps/handling/jar.mn
@@ -1,10 +1,10 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 toolkit.jar:
 % content mozapps %content/mozapps/
   content/mozapps/handling/handler.css                          (content/handler.css)
   content/mozapps/handling/handler.xml                          (content/handler.xml)
-  content/mozapps/handling/dialog.xul                           (content/dialog.xul)
+*  content/mozapps/handling/dialog.xul                           (content/dialog.xul)
   content/mozapps/handling/dialog.js                            (content/dialog.js)

Comment 17

[Russell Cheung]

Hi, I see this bug is back to unassigned. I'm new here but would be happy to pick this up if this requires work.

Comment 18

[akriti verma]

Hi, I am interested in working on this bug.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

Paolo, you cleared the review flag here for adw without explanation, yet adw is still set as the mentor for this bug. Is someone from your team willing to review and/or mentor the patches that have been sitting here for months?

Comment 20

[:Paolo Amadini]

Since Drew hasn't been active on this bug, you're right that he should probably not be listed as a mentor.

I'll ask if anyone on fx-team wants to take on mentorship.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Something to keep in mind as you work on patches is that it is preferable to combine multiple changes in a single patch.

The only issue I have with the proposed change is that we are trying to move away from preprocessed files to using AppConstants at runtime. In this case I would recommend swapping the order in dialog.js instead of dialog.xul. In dialog.initialize() you could check for AppConstants.platform == "win" and then swap the buttons.

Comment 22

[Jayati Shrivastava]

Hi, may I work on this?

Comment 23

[Jayati Shrivastava]

I think this has been fixed https://searchfox.org/mozilla-central/source/toolkit/mozapps/handling/content/dialog.xhtml#48
However, if this needs to be moved to dialog.js, I'll work on it.
Please confirm. Thank you

Comment 24

[Gian-Carlo Pascutto [:gcp]]

This was indeed fixed by bug 1580646.