Last Comment Bug 767289 - [New Tab page] The :hover border should follow the dragImage instead of staying with the thumbnail tile
: [New Tab page] The :hover border should follow the dragImage instead of stayi...
Status: RESOLVED FIXED
[good first bug][mentor=ttaubert][lan...
: polish
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Owen Coutts [:tallOwen]
:
:
Mentors:
Depends on:
Blocks: 729878
  Show dependency treegraph
 
Reported: 2012-06-22 01:22 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-07-04 14:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A patch to fix part 2 as outlined by ttaubert (1.19 KB, patch)
2012-07-03 12:49 PDT, Owen Coutts [:tallOwen]
no flags Details | Diff | Splinter Review
Update patch (1.40 KB, patch)
2012-07-03 17:40 PDT, Owen Coutts [:tallOwen]
no flags Details | Diff | Splinter Review
Patch with only 'dragged' attribute (1.21 KB, patch)
2012-07-04 08:21 PDT, Owen Coutts [:tallOwen]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch with feedback/improved styling (1.44 KB, patch)
2012-07-04 09:09 PDT, Owen Coutts [:tallOwen]
no flags Details | Diff | Splinter Review
Added css for all themes (2.29 KB, patch)
2012-07-04 09:37 PDT, Owen Coutts [:tallOwen]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (2.66 KB, patch)
2012-07-04 10:13 PDT, Owen Coutts [:tallOwen]
no flags Details | Diff | Splinter Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-06-22 01:22:18 PDT
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] 2012-06-23 22:42:56 PDT
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] 2012-06-24 15:56:19 PDT
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] 2012-07-03 12:49:52 PDT
Created attachment 638834 [details] [diff] [review]
A patch to fix part 2 as outlined by ttaubert
Comment 4 Owen Coutts [:tallOwen] 2012-07-03 17:39:03 PDT
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] 2012-07-03 17:40:50 PDT
Created attachment 638924 [details] [diff] [review]
Update patch

The first patch didn't properly remove the 'dragged-cell' attribute.
Comment 6 Tim Taubert [:ttaubert] 2012-07-04 05:09:30 PDT
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] 2012-07-04 08:21:30 PDT
Created attachment 639106 [details] [diff] [review]
Patch with only 'dragged' attribute
Comment 8 Tim Taubert [:ttaubert] 2012-07-04 08:31:08 PDT
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] 2012-07-04 09:09:31 PDT
Created attachment 639121 [details] [diff] [review]
Patch with feedback/improved styling

Thanks for the feedback!
Comment 10 Tim Taubert [:ttaubert] 2012-07-04 09:30:43 PDT
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] 2012-07-04 09:37:50 PDT
Created attachment 639126 [details] [diff] [review]
Added css for all themes
Comment 12 Tim Taubert [:ttaubert] 2012-07-04 09:47:26 PDT
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] 2012-07-04 10:13:26 PDT
Created attachment 639135 [details] [diff] [review]
Patch for checkin
Comment 14 Tim Taubert [:ttaubert] 2012-07-04 10:21:23 PDT
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] 2012-07-04 14:29:27 PDT
https://hg.mozilla.org/mozilla-central/rev/1ec7439ba477

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