767289 - [New Tab page] The :hover border should follow the dragImage instead of staying with the thumbnail tile

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Open the new tab page
Hover over a thumbnail
Notice the box-shadow and the darker border
Drag the thumbnail

Expected:
The border should move with the drag image.

Actual:
Notice that the box-shadow moves with the drag image, but the border stays still.

Tim, can you add some file references and steps for somebody to fix this?

Comment 1

[Atul Jangra [:atuljangra]]

I am waiting for Tim's comment on this.
Regarding the bug, I think border is *supposed* to be there, because as we drag a thumbnail, the next thumbnail comes in the place of first one. So do we really need to make the border move?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Currently, the cell has a border that gets darker when a site is hovered. The site itself has a box-shadow. Ideally, the site should have the border *and* the box-shadow but that's not possible because of bug 480888 - that's why I took this workaround.

There seems to be some activity in bug 480888 recently, maybe it'll get fixed soon? There are two options here:

1) Wait until bug 480888 is fixed and move the border to .newtab-site.

2) Let the border-color return to normal when we started dragging site. We could do this by setting the 'dragged' attribute for .newtab-cell as well like here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/drag.js#43

We could of course do [2] and then [1] when it's really fixed.

Comment 3

[Owen Coutts [:tallOwen]]

Attachment: A patch to fix part 2 as outlined by ttaubert

Comment 4

[Owen Coutts [:tallOwen]]

Comment on attachment 638834 [details] [diff] [review]
A patch to fix part 2 as outlined by ttaubert

diff -r 47f814827db6 browser/base/content/newtab/drag.js
--- a/browser/base/content/newtab/drag.js	Tue Jun 19 16:25:56 2012 -0700
+++ b/browser/base/content/newtab/drag.js	Tue Jul 03 20:35:53 2012 -0400
@@ -42,6 +42,8 @@
     for (let i = 0; i < nodes.length; i++)
       nodes[i].setAttribute("dragged", "true");
 
+    aSite.node.parentNode.setAttribute("dragged-cell", "true");
+
     this._setDragData(aSite, aEvent);
 
     // Store the cursor offset.
@@ -88,6 +90,13 @@
    * @param aEvent The 'dragend' event.
    */
   end: function Drag_end(aSite, aEvent) {
+    let dragged_cells = document.querySelectorAll(".newtab-cell[dragged-cell]");
+    for (let i = 0; i < dragged_cells.length; i++) {
+      if (dragged_cells[i].hasAttribute("dragged-cell")) {
+        dragged_cells[i].removeAttribute("dragged-cell");
+      }
+    }
+
     let nodes = aSite.node.parentNode.querySelectorAll("[dragged]");
     for (let i = 0; i < nodes.length; i++)
       nodes[i].removeAttribute("dragged");
diff -r 47f814827db6 browser/themes/pinstripe/newtab/newTab.css
--- a/browser/themes/pinstripe/newtab/newTab.css	Tue Jun 19 16:25:56 2012 -0700
+++ b/browser/themes/pinstripe/newtab/newTab.css	Tue Jul 03 20:35:53 2012 -0400
@@ -56,7 +56,7 @@
   -moz-margin-end: 0;
 }
 
-.newtab-cell:hover:not(:empty) {
+.newtab-cell:hover:not(:empty):not([dragged-cell]) {
   border-color: rgba(8,22,37,.25) rgba(8,22,37,.27) rgba(8,22,37,.3);
 }

Comment 5

[Owen Coutts [:tallOwen]]

Attachment: Update patch

The first patch didn't properly remove the 'dragged-cell' attribute.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 638924 [details] [diff] [review]
Update patch

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

Thank you, Owen! That looks really good but we can have that a little easier, I think. Please flag me for review again when you upload a new patch.

::: browser/base/content/newtab/drag.js
@@ +42,4 @@
>      for (let i = 0; i < nodes.length; i++)
>        nodes[i].setAttribute("dragged", "true");
>  
> +    aSite.node.parentNode.setAttribute("dragged-cell", "true");

I think we should just re-use the "dragged" attribute so that we don't need to take extra care of it being removed in Drag.end().

Can you please define a variable (like 'cell' or 'parentCell') for 'aSite.node.parentNode' before the querySelectorAll() call above?

Comment 7

[Owen Coutts [:tallOwen]]

Attachment: Patch with only 'dragged' attribute

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 639106 [details] [diff] [review]
Patch with only 'dragged' attribute

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

Thanks, Owen. I'll give you feedback+ because the patch looks really good but I'd like two small changes to be made before we can land it.

::: browser/base/content/newtab/drag.js
@@ +42,4 @@
>      for (let i = 0; i < nodes.length; i++)
>        nodes[i].setAttribute("dragged", "true");
>  
> +    let parentCell = aSite.node.parentNode;

Please define this variable above the for-loop so that you can use it for the querySelectorAll() call as well.

@@ +91,4 @@
>     * @param aEvent The 'dragend' event.
>     */
>    end: function Drag_end(aSite, aEvent) {
> +    let nodes = document.querySelectorAll("[dragged]");

|let nodes = gGrid.node.querySelectorAll("[dragged]")| would be a little better here because that would constrain the selector to grid descendants only.

Comment 9

[Owen Coutts [:tallOwen]]

Attachment: Patch with feedback/improved styling

Thanks for the feedback!

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 639121 [details] [diff] [review]
Patch with feedback/improved styling

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

Oops, I didn't realized that the style change was done in a theme-specific CSS file. Please also change the newTab.css files in themes/winstrip and themes/gnomestrip. Sorry.

Comment 11

[Owen Coutts [:tallOwen]]

Attachment: Added css for all themes

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 639126 [details] [diff] [review]
Added css for all themes

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

Thank you, great work! Can you please upload a new patch that is prepared for checkin? This post describes how it's done:

http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 13

[Owen Coutts [:tallOwen]]

Attachment: Patch for checkin

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Congratulations on landing your first patch, Owen! I pushed it to the fx-team repo, from there it'll be merged to mozilla-central in a few days and will then be available in Nightly. Thank you for your contribution!

https://hg.mozilla.org/integration/fx-team/rev/1ec7439ba477

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1ec7439ba477