Last Comment Bug 619055 - Text cursor should be active only when mouseover() the exact area of the group name field
: Text cursor should be active only when mouseover() the exact area of the grou...
Status: RESOLVED FIXED
[4b8][visual][polish][good first bug]
: uiwanted
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P4 normal
: Future
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 671243
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-14 06:23 PST by Maniac Vlad Florin (:vladmaniac)
Modified: 2016-04-12 14:00 PDT (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch to make only the textbox-hover use the text cursor (504 bytes, patch)
2011-02-07 11:17 PST, Harry
no flags Details | Diff | Splinter Review
Patch to make only the textbox-hover use the text cursor (2) (5.35 KB, patch)
2011-02-07 11:21 PST, Harry
no flags Details | Diff | Splinter Review
Patch to make only the textbox-hover use the text cursor (3) (5.59 KB, patch)
2011-02-07 11:48 PST, Harry
ian: review-
faaborg: ui‑review-
Details | Diff | Splinter Review
Patch to make only the textbox-hover use the text cursor (4) (4.51 KB, patch)
2011-02-09 12:33 PST, Harry
mitcho: feedback-
Details | Diff | Splinter Review

Description Maniac Vlad Florin (:vladmaniac) 2010-12-14 06:23:54 PST
Build ID: 

Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101213 Firefox/4.0b8pre 

The text cursor triggers on hover mouse event for the whole line in which the name text field exists. It should appear only when hovering the exact area of text. 

For example, in the area outside the group's name field, the positioning cursor 
should appear instead of the text cursor.
Comment 1 Michael Yoshitaka Erlewine [:mitcho] 2011-01-20 15:28:13 PST
Punting. This should be a trivial CSS change, though, so we'll consider a patch if one is created before beta 11.
Comment 2 Harry 2011-02-07 11:17:49 PST
Created attachment 510330 [details] [diff] [review]
Patch to make only the textbox-hover use the text cursor

Okay, I don't have the excuse of first patch anymore. Second, then :)

My chosen solution makes the textbox use the text cursor, highlight the text box and not be dragable, leaving the rest of the title bar using the move cursor, not highlighting the textbox and be dragable. Admittedly, this represents a slight change in functionality outside the remits of this bug and bug#627229, since the textbox was previously dragable if it was unfocussed.

Given that the cursors are now clear, however, I feel that this is probably the functionality expected by a user. They retain use of most of the rest of the group - signalled by the move cursor - if they wish to move it around.

Grep-ing around, I found no other remaining references to title-container or title-shield, so I went ahead and removed them. I have tested the patch on my own Windows 7 setup: the changes to the Linux and Mac themes are untested but follow a very similar pattern.

Thanks!
Comment 3 Harry 2011-02-07 11:21:22 PST
Created attachment 510332 [details] [diff] [review]
Patch to make only the textbox-hover use the text cursor (2)

The patch I intended to attach.
Comment 4 Harry 2011-02-07 11:48:26 PST
Created attachment 510342 [details] [diff] [review]
Patch to make only the textbox-hover use the text cursor (3)

Add a couple of changes that didn't make it into the last patch. (Sorry!)

Test for bug #618816 still broken - not sure how to fix it - but this patch doesn't affect the code added in that revision AFAICT.
Comment 5 Ian Gilman [:iangilman] 2011-02-07 14:32:19 PST
Comment on attachment 510342 [details] [diff] [review]
Patch to make only the textbox-hover use the text cursor (3)

Interesting solution; certainly cleans up the code a good deal. I can see pros and cons to the UX impact: it makes group name editing easier, but group moving harder. While it's true that you can drag the group from any portion, not just the "title bar", I suspect that top area is the natural place for people to try. With this patch and a long title in a narrow group, no portion of the "title bar" can be dragged on for moving the group.

I don't know, but I imagine people naming their groups once and not spending a lot of time editing or renaming them. I'm also concerned about the potential for accidental name changes. At any rate, assigned to Faaborg for UI review.

If we do end up using this patch, there are still some things to sort out: 

* If you click and drag the first time you edit a name, the name just gets fully selected rather than just the portion you were dragging over. This is because we auto-select on that first click, which is what we want if it's just a single-click, but not a click drag (see the awesome bar)

* If you select part of a name and then defocus the name field, there's no indication that any portion of the name is selected, but it still is. If you click and drag into it refocus, you end up dragging the selected portion of the text around. Blur should deselect the text as well.

* The box around the edit text area should probably be a bit stronger to help reinforce that the area does something different (though I'd still like it to go away when the text box is not focused and the cursor's not over it)

* I was able to get into text editing mode without the box around the text area visible... not sure how I did that.

* Probably other unintended consequences?

I think my recommendation is that we don't tackle this for Fx4, though I certainly appreciate the attention to the bug, Harry!
Comment 6 Alex Faaborg [:faaborg] (Firefox UX) 2011-02-07 20:05:36 PST
Comment on attachment 510342 [details] [diff] [review]
Patch to make only the textbox-hover use the text cursor (3)

Turning down for all of these concerns that Ian had:

>it makes group name editing easier, but group moving
>harder. While it's true that you can drag the group from any portion, not just
>the "title bar", I suspect that top area is the natural place for people to
>try. With this patch and a long title in a narrow group, no portion of the
>"title bar" can be dragged on for moving the group.
>
>I don't know, but I imagine people naming their groups once and not spending a
>lot of time editing or renaming them. I'm also concerned about the potential
>for accidental name changes.

If we can get the cursor to more accurately display based on the bounds of the text field, but not impact downclick-drag operations, that would be the ideal way to address this bug.
Comment 7 Harry 2011-02-09 12:33:37 PST
Created attachment 511146 [details] [diff] [review]
Patch to make only the textbox-hover use the text cursor (4)

Okay, last go at this one for now, I promise.

This patch does nothing except resolve the bug. That is, the whole titlebar remains draggable, but now only the text-area portion causes the visual signalling for the text area (cursor and hover graphic).

I realise that some of Ian's concerns about the setup in general are therefore not tackled e.g. strength of the border, but I think it's a useful patch for someone else to take forward nonetheless.

Works on my Windows 7 setup.

Regards.
Comment 8 Michael Yoshitaka Erlewine [:mitcho] 2011-02-09 22:43:16 PST
Comment on attachment 511146 [details] [diff] [review]
Patch to make only the textbox-hover use the text cursor (4)

>+	.mouseover(function(e) {
>+	  self.$title.addClass("name-hover");
>+	})
>+	.mouseout(function(e) {
>+	  self.$title.removeClass("name-hover");
>+	});

nit: These are tab characters. Please use spaces instead.

>+    this.$title.css({width: w});
>+    this.$titleShield.css({width: w+21});

Where did this 21 value come from? Shouldn't we let whatever this value is be themeable?

> .title-shield {
>   position: absolute;
>-  left: 0;
>+  left: 5;
>   top: 0;
>   width: 100%;
>   height: 100%;
>   z-index: 10;
>+  cursor: text;
> }

Changing the left value here seems dangerous for RTL languages. Please check. Also, even if this is what we want to do, perhaps we should put it in the platform-specific CSS files.

In general: adding this name-hover class doesn't sit well with me, though removing title-container strikes me as a good idea. What if instead of name-hover the title-shield div actually takes the input as its child, and we set pointer-events:none on the title-shield? Would that work? (Not necessarily a good idea!)

Reference: https://developer.mozilla.org/en/CSS/pointer-events

I agree with Ian, though, that we should formally punt on this for fx4. :(

Thanks for looking into this!
Comment 9 Harry 2011-02-10 09:21:32 PST
> nit: These are tab characters. Please use spaces instead.

Of course, my bad.

> >+    this.$title.css({width: w});
> >+    this.$titleShield.css({width: w+21});
> 
> Where did this 21 value come from? Shouldn't we let whatever this value is be
> themeable?

I'd have a think about that one and get back to you, come to mention it.

> > .title-shield {
> >   position: absolute;
> >-  left: 0;
> >+  left: 5;
> >   top: 0;
> >   width: 100%;
> >   height: 100%;
> >   z-index: 10;
> >+  cursor: text;
> > }
> 
> Changing the left value here seems dangerous for RTL languages. Please check.
> Also, even if this is what we want to do, perhaps we should put it in the
> platform-specific CSS files.

Works for me: CSS for multiple platforms / locales is not something I've done before, alas.

> In general: adding this name-hover class doesn't sit well with me, though
> removing title-container strikes me as a good idea. What if instead of
> name-hover the title-shield div actually takes the input as its child, and we
> set pointer-events:none on the title-shield? Would that work? (Not necessarily
> a good idea!)

My original intent was to have the input as the child of the shield, but I couldn't find a way to z-index the div in front of the input. There may be a way to do that, but I don't know of it (which isn't saying much :P). The title-shield has to receive mouse events to enable dragging, which I guess if why it's there in the first place.

Anyway, it was fun to have a play around with some actual code for a change. Maybe once ff4 is released I'll have another look at fixing this, assuming someone doesn't get there before me.

Regards.
Comment 10 Ian Gilman [:iangilman] 2011-02-14 16:33:13 PST
(In reply to comment #9)
> Anyway, it was fun to have a play around with some actual code for a change.
> Maybe once ff4 is released I'll have another look at fixing this, assuming
> someone doesn't get there before me.

Thanks for helping out! I hope to see you again once the FF4 dust settles. :)
Comment 11 Tim Taubert [:ttaubert] 2011-10-11 02:42:59 PDT
Fixed by bug 671243.

Note You need to log in before you can comment on or make changes to this bug.