440150 - Library window divider between content list and bookmarks too thin

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4) Gecko/20030624

In the contextual bookmarks window/library, the divider between the list of items on the left and the content of them on the right is only one pixel wide.  This is an adjustable divider, so in order to move it the user must finagle their cursor so that it is above the one-pixel column that allows them to drag.

Strangely, in this same window, the column titles in the right section does provide adequate hit area for dragging.

Reproducible: Always

Steps to Reproduce:
1. Open Library
2. Try to adjust the width of the bookmarks list
3. Can you get it?  Try again...

Actual Results:  
Hit area 1 pixel wide

Expected Results:  
Hit area at least 5 pixels wide

Comment 1

[[:Aleksej]]

Attachment: Test on GTK/Linux

Looks like 3, maybe 4, pixels wide to me. No problem hitting.
First 2 tries are using a graphics tablet, third try using a mouse.

Video made with
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a1pre) Gecko/2008061902 Minefield/3.1a1pre

Feels the same with
Mozilla/5.0 (X11; U; Linux i686; eo; rv:1.9) Gecko/2008052912 Firefox/3.0

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

I'm on a OSX and just measured one pixel.  Could it be there's a difference for Mac?

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Kevin: is this a CSS thing?

Comment 4

[Kevin Gerich]

Yes it is. I'll see what I can do.

Comment 5

[(mostly gone) XtC4UaLL [:xtc4uall]]

any update on this, Kevin?

Comment 6

[Marco Bonardo [:mak]]

we should do something similar to Windows Aero, still show a 1px divider, but make it a better hit target. see http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/places/organizer-aero.css#39

The code to modify is
http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/places/organizer.css#66 (it may actually end up being trivial)

Comment 7

[Surya Seetharaman]

Hello I am a newbie and I would like to work on this bug. Could someone kindly guide me?

Comment 8

[Surya Seetharaman]

Attachment: bug440150

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8444398 [details] [diff] [review]
bug440150

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

::: browser/themes/osx/places/organizer.css
@@ +66,5 @@
>  #placesView > splitter {
>    -moz-border-start: none !important;
>    -moz-border-end: 1px solid #bdbdbd;
>    min-width: 1px;
> +  width: 3px;

just by setting this, I see 2px of the splitter background color across all of the splitter, and that looks bad.

Please copy the trick we did on Windows, add -moz-margin-start: -3px; and position: relative; That should keep the 3px handling area but hide the background. Please verify that, it seems to work fine here but double checking it will be welcome :)

@@ +71,1 @@
>    background-image: none !important;       

while here please remove the trailing spaces from this line.

Comment 10

[Surya Seetharaman]

Attachment: bug440150

I have done the necessary changes to the bug. Also kindly let me know what  I should do to get the bug assigned to me.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Surya Seetharaman from comment #10)
> Also kindly let me know what 
> I should do to get the bug assigned to me.

Oh sorry, I should have already done that. Usually, just ask :)

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8454526 [details] [diff] [review]
bug440150

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

It is definitely better, I can't tell if 3px is enough for most people, but in case we figure it's not we should probably enlarge that on all platforms in a separate bug (so far it worked decently on Windows)

r=me with the following small fix and a proper commit message like "Bug 123456 - description. r=reviewer"

::: browser/themes/osx/places/organizer.css
@@ +69,5 @@
>    min-width: 1px;
> +  width: 3px;
> +  -moz-margin-start: -3px;
> +  position: relative;
> +  background-image: none !important;}

looks like you also removed the \n from here :)
Please bring back the brace to its own line, you should have only removed the trailing whitespaces from there.

Comment 13

[Surya Seetharaman]

Thank you for the review! :) 

> looks like you also removed the \n from here :)
> Please bring back the brace to its own line, you should have only removed the trailing whitespaces from there.


Should I submit another patch to bring the braces back to its new position or is it enough that I update that when making changes to the repository?(In reply to Marco Bonardo [:mak] from comment #12)

Comment 14

[Marco Bonardo [:mak]]

(In reply to Surya Seetharaman from comment #13)
> Should I submit another patch to bring the braces back to its new position
> or is it enough that I update that when making changes to the repository?

Either would work, but if you don't have level 3 access to the tree you will have to set the checkin-needed keyword to ask someone else to push for you. In such a case you should post a ready-for-checkin patch.

Comment 15

[Surya Seetharaman]

Attachment: bug440150 ready-for-checkin patch.

This is the ready-for-checkin patch. Could you kindly push it into the tree as I din't have the access.

Comment 16

[Surya Seetharaman]

(In reply to Marco Bonardo [:mak] from comment #14)

> Either would work, but if you don't have level 3 access to the tree you will
> have to set the checkin-needed keyword to ask someone else to push for you.
> In such a case you should post a ready-for-checkin patch.

I have posted the ready-for-checkin patch in comment #15. Could you please push it into the tree as I don't think I have access. Kindly let me know if there is anything I have to do for this.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8456698 [details] [diff] [review]
bug440150 ready-for-checkin patch.

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

Sorry this is still missing the check-in comment, you can set it with hg qref -m "comment" or hg qref -e (that should open the default text editor, where you can put the comment)

Then you can just set checkin-needed into the bug's keyword field.

::: browser/themes/osx/places/organizer.css
@@ +70,5 @@
> +  width: 3px;
> +  -moz-margin-start: -3px;
> +  position: relative;
> +  background-image: none !important;
> +  }

indentation is still wrong, the brace should not be indented.

Comment 18

[Marco Bonardo [:mak]]

(that commands are valid if you are using mercurial queues https://developer.mozilla.org/en-US/docs/Mercurial_Queues)

Comment 19

[Surya Seetharaman]

Attachment: bug440150 ready-for-checkin patch.

Here is the ready for check-in  patch after making the necessary changes and commenting . Hope it is proper.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8456852 [details] [diff] [review]
bug440150 ready-for-checkin patch.

># HG changeset patch
># Parent 51b428be6213b7d70f9fe316f61d5a114bb22c6b
># User tssurya <[e-mail]>
># Date:16/07/2014
>
>#ready-for-check-in patch.
>#requesting check-in for this patch.
>

The patch is fine, the checkin-comment is still wrong. It should be

Bug 440150 - Library window divider between content list and bookmarks too thin. r=mak

Btw, sheriffs can fix it on push for this time. checkin-needed keyword is enough, no need to flag checkin? (we usually use that for multi-patch bugs)

Comment 21

[Surya Seetharaman]

Attachment: bug440150 ready-for-checkin patch.

Comment 22

[Surya Seetharaman]

(In reply to Marco Bonardo [:mak] from comment #20)

> 
> The patch is fine, the checkin-comment is still wrong. It should be
> 
> Bug 440150 - Library window divider between content list and bookmarks too
> thin. r=mak
> 
> Btw, sheriffs can fix it on push for this time. checkin-needed keyword is
> enough, no need to flag checkin? (we usually use that for multi-patch bugs)

Oh okay I have removed the checkin? flag. So that line in the patch is like a typical "checkin comment" I guess. I haven't commented it , as when I searched through some other patches I saw them having just written it.If I have to put it behind '#' please let me know. Thanks in advance!

Comment 23

[Marco Bonardo [:mak]]

It looks good now! thank you.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/4024d8019701

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/4024d8019701

Comment 26

[Francesca Ciceri [:madamezou]]

Hi,

I've reproduced it on Nightly Mozilla Firefox 33.0a1 (20140626) and I can confirm it's fixed in latest Nightly Firefox/34.0 ID:20140723030202 on Linux x86_64.

Cheers,
Francesca

Comment 27

[Francesca Ciceri [:madamezou]]

Argh! Wrong bug number: sorry for the noise, please ignore Comment 26.

Cheers,
Francesca