1215856 - ":" should not be selected or copied in Title field

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[YF (Yang)]

I think people generally need to copy this field as an filename or anything, and this colon added to the results leads to trouble. For example, it is an illegal character for path in Windows.


I think there are several solutions:
1. Automatic clipping the results when hotkeys or context menus.
2. Automatically cancel its selection state when selected event, which may result in an inconsistent jump.
3. Avoid it is selected. I hope this can be done, and not by a single element, so as not to affect the field area.


p.s. It should also be allowed to be l10n.

Comment 1

[Pushkar]

I am new to open source community and want to solve this bug as my first bug.Can you please guide me as to how can I proceed.

Comment 2

[YF (Yang)]

https://developer.mozilla.org/en-US/docs/Introduction


The code:
https://hg.mozilla.org/mozilla-central/annotate/cdcd33fd6e39cd12feb5bb11951e1c981a04bd86/browser/base/content/pageinfo/pageInfo.js#l533


I look forward to helping you, if you need more info.

Comment 3

[Deepjyoti Mondal]

Attachment: Screenshot from 2016-07-30 21-12-28.png

Since there has been no update on this bug for a long time, I thought of picking it up. What if there is no ':' at the first place?

Comment 4

[YF (Yang)]

Also good, but it would be better if there is a sense of depth or shading for the sub-information.

Comment 5

[Deepjyoti Mondal]

Well the title is already bolder than the sub-information.Please let me know what else do you suggest in order to add a sense of depth to the sub-information while omitting the ':' from the tite?

Comment 6

[YF (Yang)]

Like indents and triangle arrow, or frame border.

Comment 7

[Deepjyoti Mondal]

Attachment: Screenshot from 2016-08-01 15-15-27.png

Added border around the subsection

Comment 8

[Deepjyoti Mondal]

Attachment: Bug1215856.patch

Patch for the above.

Comment 9

[YF (Yang)]

(In reply to Deepjyoti Mondal from comment #7)
> Created attachment 8776578 [details]
> Screenshot from 2016-08-01 15-15-27.png
> 
> Added border around the subsection

It is not what I think the frame border (like webpage frame).


I'm not a reviewer.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch

Comment 10

[Deepjyoti Mondal]

Sorry for bothering you with review.

It seems I confused frame border with ordinary css border. Can you please provide me with an example of frame border so that I can continue with the patch.

Comment 11

[YF (Yang)]

Attachment: Screenshot from 2016-07-30 21-12-28 - Modified.png

Note that my ideas is not an order.


I like the UI (sub-information) like screenshot.

A frame border like webpage's border in Microsoft IE, but it may not be common in Firefox.

Comment 12

[Deepjyoti Mondal]

Attachment: bug1215856v2.patch

I modified the appearance of the subsection below the title field and the title field itself to make it look like the screen shot in the previous post.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8777942 [details] [diff] [review]
bug1215856v2.patch

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

I'm not sure that the proposal is the right choice here. Stephen, can you weigh in or find someone to weigh in on what that section of the page info dialog should look like?

::: browser/base/content/pageinfo/pageInfo.js
@@ +533,4 @@
>  function makeGeneralTab(metaViewRows, docInfo)
>  {
>    var title = (docInfo.title) ? gBundle.getFormattedString("pageTitle", [docInfo.title]) : gBundle.getString("noPageTitle");
> +  document.getElementById("titletext").label = title.slice(0,title.length-1);

The correct change would be to edit the l10n strings to remove the :.

Comment 14

[Deepjyoti Mondal]

Can you please point me out where can I find the l10n strings?

Comment 15

[Dave Townsend [:mossop]]

Here: https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/pageInfo.properties#8
Changing strings means changing the string key too but let's wait on a UX response first.

Comment 16

[Deepjyoti Mondal]

Okay

Comment 17

[Stephen Horlander [:shorlander]]

(In reply to Dave Townsend [:mossop] from comment #13)
> I'm not sure that the proposal is the right choice here. Stephen, can you
> weigh in or find someone to weigh in on what that section of the page info
> dialog should look like?

We could just make the title consistent with the other information. E.g.:

Title:   Some title goes here
Address: http://address.address
Type:    text/html
etc.

Comment 18

[Deepjyoti Mondal]

Should the title remain bold as it is now or should it be normal like the other information.

Comment 19

[Deepjyoti Mondal]

Attachment: bug1215856_v2.patch

This is a test patch.I have tried to make the title consistent with the other fields.

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8792659 [details] [diff] [review]
bug1215856_v2.patch

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

::: browser/locales/en-US/chrome/browser/pageInfo.properties
@@ +6,4 @@
>  pageInfo.frame.title=Frame Info - %S
>  
>  noPageTitle=Untitled Page:
> +pageTitle=%S

This is unnecessary now isn't it?

Comment 21

[Deepjyoti Mondal]

Attachment: bug1215856_v2.patch

modified my previous patch.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 8792949 [details] [diff] [review]
bug1215856_v2.patch

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

Looks good, thanks for the patch.

Comment 23

[Stephen Horlander [:shorlander]]

(In reply to Deepjyoti Mondal from comment #18)
> Should the title remain bold as it is now or should it be normal like the
> other information.

Since it's just another piece of site information I would make it the same normal weight as the rest.

Comment 24

[Deepjyoti Mondal]

Most of the issues mentioned in bug 1308737 are solved by the previous patch (8792949) except the Title Case issue. Since most of the work has already been done in this bug, will it not be better to extend the previous patch and address the Title Case issue here itself ?

Comment 25

[Dave Townsend [:mossop]]

(In reply to Deepjyoti Mondal from comment #24)
> Most of the issues mentioned in bug 1308737 are solved by the previous patch
> (8792949) except the Title Case issue. Since most of the work has already
> been done in this bug, will it not be better to extend the previous patch
> and address the Title Case issue here itself ?

It's unclear whether bug 1308737 is a workable bug right now, it would require designs to move forward. We should just land this patch as it is ready to go.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd4ac14010d
"":" should not be selected or copied in Title field". r=dtownsend

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2dd4ac14010d

Comment 28

[Md. Majedul islam]

I have reproduced this bug with Nightly 44.0a1 (2015-10-17) on ubuntu 16.10,64 Bit!

This bug's fix is verified with latest Beta !

Build    ID : 20170216105119
User  Agent : Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0

[testday-20170217]