708550 - Cannot copy version string from "About Thunderbird" dialogue window (regression)

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Thomas D. (:thomas8)]

Nitfix:

STR

1) Help > About TB
2) Try to copy version string (for bug reports, support requests etc.), e.g. for Daily: "11.0a1 (2011-12-08)"

Actual
- cannot copy version string (not even with mouse)

Expected
- at least allow mouse-selection of version string for copying
- better: make version string keyboard-accessible (focus and selection)
- this was working in previous versions of TB (regression)
- this is working in FF 8 and FF Nightly (so it's us who broke it, or didn't fix it)

This should be really trivial to fix, perhaps just display the version string in an invisible text field, so that standard methods for keyboard and mouse access are available (at least we should not be worse than FF).

FTR: This is NOT a duplicate of Bug 572769 (where this was still working), and going to Help > Troubleshooting (about:support) instead is NOT a solution for this bug (hell no, I don't want to search for a simple version string between dozens of entries, and then copy an unreadable User Agent monster string like "Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111205 Thunderbird/11.0a1").

Comment 1

[Thomas D. (:thomas8)]

While we're here, we should also fix focus behaviour and indication:

Currently, initial focus is always on "Apply downloaded update" button, or on the "TB is up to date" label, so first TAB will always take you to mozilla.org link.
However, we are inconsistent about the focus ring on the button: Initially, no focus ring (but button has focus), but: TAB, then Shift+TAB -> Button has focus AND focus ring. We should decide if the button should have a focus ring (or not) when focused (probably yes), and then make the behaviour consistent for initial and subsequent manual focus.

So starting out from current initial focus which is always on "Apply..." button or "...up to date" label, *Shift+TAB* from there should always focus and select the version string right above the button location (proposed solution for this bug). Thus, we are not changing the forward-TAB sequence, AND we are helpful and well-behaved about focusing and selecting the version string with first Shift+TAB for helpful folks who'd like to copy that. Simple. Nothing extraordinary here.

Comment 2

[rsx11m]

Is the copy functionality supposed to work for the pre-release builds only?
I can't mark the "8.0" string in the current release already... (Windows 7)

Comment 3

[David E. Ross]

Users should be able to select and copy all text within the About Mozilla Thunderbird popup.  

While composing bug #709489, I tried to copy the text for the links at the bottom of the popup; this was not possible.  Instead, I had to contrive small windows for both Thunderbird and SeaMonkey so that I could see the text in the popup while manually typing the text into the Bugzilla page in SeaMonkey.

Comment 4

[Wayne Mery (:wsmwk)]

FWIW, works in firefox

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk) from comment #4)
> FWIW, works in firefox

code can be copied from bug 598169

Comment 6

[sakshi]

Hello,

I would like to work on this bug.

Comment 7

[sakshi]

Attachment: WIP

Comment 8

[sakshi]

Attachment: WIP

Comment 9

[Thomas D. (:thomas8)]

Comment on attachment 715854 [details] [diff] [review]
WIP

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

For a start, we want to do the same thing here as FF, so this looks good at first sight (I'll check the details later). However, there's one line which breaks the patch.

::: mail/base/content/aboutDialog.xul
@@ +40,5 @@
>    <vbox id="aboutDialogContainer">
>      <hbox id="clientBox">
>        <vbox id="leftBox" flex="1"/>
>        <vbox id="rightBox" flex="1">
> +#expand <label id="version"> 17.0.2</label>

Pls don't hard-code the version number, you need to keep the variable, in analogy to the FF patch.

Comment 10

[sakshi]

Yes, I tried that. But when i tried __MOZ_APP_VERSION__ it was directly printing this on the ' About thunderbird' dialog

Comment 11

[Richard Marti (:Paenglab)]

You have patched the files directly in omni.ja to test?
__MOZ_APP_VERSION__ will be exchanged during build (compiling) with the correct string.

Comment 12

[sakshi]

Attachment: WIP

Comment 13

[rsx11m]

(In reply to Richard Marti [:Paenglab] from comment #11)
> __MOZ_APP_VERSION__ will be exchanged during build (compiling) with the
> correct string.

Yes, everything starting with a "#something" is a pre-processor directive and will thus only be effective when you create your own build. Thus, you'll see the actual version number hardcoded in the omni.ja's XUL file, which is different from the given source found in the repository.

Also, flagging the patch for "review?" is good, but you have to pick a reviewer (module owner or peer) to assign the review.

Comment 14

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #13)
> Also, flagging the patch for "review?" is good, but you have to pick a
> reviewer (module owner or peer) to assign the review.

o In the spirit of the new New Release and Governance Model (1) which requires more collaboration and community-driven involvment,
o encouraged by respective statements in a recent status meeting (2), and
o emboldened by the fast & successful cooperative review experience in bug 581470 (also involving me and Sakshi)
I hereby submit that I'd like to snatch the review for this simple patch! :)

Which potentially spares some valuable time and energy of main reviewers for higher purposes. However, I'll gladly request an additional review from them when I'm done.

(1) https://wiki.mozilla.org/Thunderbird/Proposal:_New_Release_and_Governance_Model
(2) https://wiki.mozilla.org/Thunderbird/StatusMeetings/2013-02-05#Question_Time

Comment 15

[rsx11m]

Sure, no objections from this end. :-)

I was mostly prompted to that comment given that there is no name associated with Sakashi's review request, thus wanted to point out that it might get lost without naming a specific reviewer.

Comment 16

[Thomas D. (:thomas8)]

Comment on attachment 715951 [details] [diff] [review]
WIP

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

Sakshi, thanks for the quick patch :)

As announced and explained in my comment 14, I'll snatch the review for this trivial patch which is identical to FF patch of attachment 539592 [details] [diff] [review] :)

r=me with the following nit fixed:

::: mail/base/content/aboutDialog.js
@@ +36,5 @@
>    let version = Services.appinfo.version;
>    if (/a\d+(pre)?$/.test(version)) {
>      let buildID = Services.appinfo.appBuildID;
>      let buildDate = buildID.slice(0,4) + "-" + buildID.slice(4,6) + "-" + buildID.slice(6,8);
> +    document.getElementById("version").textContent += " (" + buildDate + ")";

Looking at the source (1) in mxr, this line has already been fixed (perhaps recently - pls ensure you're patching against the most recent files), so you can remove aboutDialog.js entirely from your patch.

(1) http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/aboutDialog.js#40

Comment 17

[rsx11m]

(In reply to Thomas D. from comment #16)
> Looking at the source (1) in mxr, this line has already been fixed (perhaps
> recently - pls ensure you're patching against the most recent files), so you
> can remove aboutDialog.js entirely from your patch.

Eh, yes - it was indeed fixed, but in mozilla-central, not in comm-central...  ;-)

> (1)
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/aboutDialog.js#40

vs.

> http://mxr.mozilla.org/comm-central/source/mail/base/content/aboutDialog.js#40

However, the patch doesn't apply for me against comm-central even though it should:

> % patch -p1 < about.patch 
> patching file mail/base/content/aboutDialog.css
> patching file mail/base/content/aboutDialog.js
> patching file mail/base/content/aboutDialog.xul
> Hunk #1 FAILED at 36.
> 1 out of 1 hunk FAILED -- saving rejects to file mail/base/content/aboutDialog.xul.rej

The reason being that it has two trailing spaces after the [+/-]#expand lines = removing those makes the patch apply cleanly for me.

Comment 18

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #17)
> (In reply to Thomas D. from comment #16)
> > Looking at the source (1) in mxr, this line has already been fixed (perhaps
> > recently - pls ensure you're patching against the most recent files), so you
> > can remove aboutDialog.js entirely from your patch.
> 
> Eh, yes - it was indeed fixed, but in mozilla-central, not in
> comm-central...  ;-)

Thanks for the correction!

It is indeed fixed in mozilla-central...
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog.js#40

...AND in comm-central's browser component (where I got confused because it's also inside comm-central, but not on the mail root):

> > (1)
> > http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/aboutDialog.js#40
> vs.

...but not yet in TB:
> > http://mxr.mozilla.org/comm-central/source/mail/base/content/aboutDialog.js#40

So aboutDialog.js needs to stay in the patch as is, but those trailing spaces in the #replace lines of aboutDialog.xul need to be removed. Thanks.

Comment 19

[rsx11m]

(In reply to Thomas D. from comment #18)
> ...AND in comm-central's browser component (where I got confused because
> it's also inside comm-central, but not on the mail root):

No, there is no browser component in comm-central itself. Everything within the mozilla/ directory is the complete mozilla-central tree, thus including Firefox's browser/ sources even though they are not used. The mozilla/ directory is created when running "python client.py checkout" after you've checked out the comm-central repository, to get the dependent content.

Note that http://hg.mozilla.org/comm-central doesn't show the mozilla/ directory when browsing the current changeset, so that's something MXR seems to do on its own when searching it.

Comment 20

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #14)
> (In reply to rsx11m from comment #13)
> o encouraged by respective statements in a recent status meeting (2), and
> (2)
> https://wiki.mozilla.org/Thunderbird/StatusMeetings/2013-02-05#Question_Time

To clarify, that "anyone" in "can anyone steal reviews" was really meant "anyone that is a submodule peer of another module", unless i'm mistaken (though i didn't make if for the meeting). We certainly don't have a policy that *anyone* can stamp review on something just because they think it's trivial.

That's not to say it can't be helpful for reviewers if someone have time to check patches work and are as expected in mozilla, especially for people not yet familiar with the procedures. However, please use the feedback flag for that instead, otherwise it can be hard for someone new to know what's really expected. It also easily get wrong review flags when someone checks the patch in.

Comment 21

[sakshi]

Attachment: Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)

Comment 22

[rsx11m]

Comment on attachment 717839 [details] [diff] [review]
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)

Sakashi, I'm changing your review request to one for feedback per comment #20.
Meaning, this will still require a review by a module peer.

Comment 23

[sakshi]

Okay. So who will be reviewing the patch?

Comment 24

[Richard Marti (:Paenglab)]

Mike Conley would be a good candidate.

Comment 25

[sakshi]

Okay. So I should set him to review the patch. His email-address?

Comment 26

[rsx11m]

Mike (type mconley to autocomplete his e-mail) was the primary reviewer already on your previous patch. Wait for Thomas' feedback and then you can request review from him (and there are more in case he won't have time to look into it).

Comment 27

[rsx11m]

(or cancel the feedback and go directly for the review, your choice.)

Comment 28

[sakshi]

That's okay. I can wait for the feedback from Thomas and then ask for review.

Comment 29

[Thomas D. (:thomas8)]

Comment on attachment 717839 [details] [diff] [review]
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)

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

The patch looks correct now.

Comment 30

[Thomas D. (:thomas8)]

Comment on attachment 717839 [details] [diff] [review]
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)

Mike, another 5-line patch by new contributor Sakshi awaiting your review in the footsteps of bug 581470, where the cooperative review process around your rapid technical review enabled that patch to land within only 14 days. Encouraged by that experience, here's Sakshi's next patch only 1 week after that. And she's eager for more as soon as this gets cleared by you :)

To ease your review, we have pre-reviewed the patch and it applies cleanly per  rsx11m's comment 17. This is the twin patch of attachment 539592 [details] [diff] [review] from FF bug 598169, where the "About Firefox" dialogue has been tweaked in exactly the same way.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 717839 [details] [diff] [review]
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)

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

Yep, this looks good to go! Thanks sakshi!

Comment 32

[rsx11m]

Sakshi, you have all approvals needed now for your patch and can add "checkin-needed" to the keywords at the top of this bug. Someone will check it into the code with the next round.

Comment 33

[Thomas D. (:thomas8)]

(In reply to rsx11m from comment #32)
> Sakshi, you have all approvals needed now for your patch and can add
> "checkin-needed" to the keywords at the top of this bug. Someone will check
> it into the code with the next round.

It needs more rights than Sakshi currently has to change keywords ;)

Comment 34

[rsx11m]

Thanks Thomas. In theory, changing the basic fields should be permitted to her given that I made her the assignee (at least this is how it used to work in bugzilla in the past, unless that got removed with some update).

Comment 35

[Wayne Mery (:wsmwk)]

(In reply to Thomas D. from comment #33)
> It needs more rights than Sakshi currently has to change keywords ;)

we can fix that if Sakshi agrees to fix 50 more bugs!

:) Just kidding. 

I don't know if privs are needed, but I have just granted them. Sakshi can now change field and confirm/close bugs.

In general, there are problems of this type for newbies, such as bug 802269

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/6282d8307215

Comment 37

[rsx11m]

Verified fixed in tinderbox build linux64/1361821896, (X11; Linux x86_64; rv:22.0) Thunderbird/22.0a1; I can mark and copy "22.0a1 (2013-02-25)" now to the clipboard, and it pastes fine into a text window.