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

RESOLVED FIXED in Firefox 16

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: tallOwen)

Tracking

({polish})

Trunk
Firefox 16
polish
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=ttaubert][lang=js][lang=css])

Attachments

(1 attachment, 5 obsolete attachments)

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?
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?
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.
(Assignee)

Comment 3

5 years ago
Created attachment 638834 [details] [diff] [review]
A patch to fix part 2 as outlined by ttaubert
Assignee: nobody → owen
(Assignee)

Updated

5 years ago
Attachment #638834 - Flags: review?(ttaubert)
(Assignee)

Comment 4

5 years ago
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);
 }
(Assignee)

Comment 5

5 years ago
Created attachment 638924 [details] [diff] [review]
Update patch

The first patch didn't properly remove the 'dragged-cell' attribute.
Attachment #638834 - Attachment is obsolete: true
Attachment #638834 - Flags: review?(ttaubert)
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?
(Assignee)

Updated

5 years ago
Attachment #638924 - Flags: review?(ttaubert)
(Assignee)

Updated

5 years ago
Attachment #638924 - Flags: review?(ttaubert)
(Assignee)

Comment 7

5 years ago
Created attachment 639106 [details] [diff] [review]
Patch with only 'dragged' attribute
Attachment #638924 - Attachment is obsolete: true
Attachment #639106 - Flags: review?(ttaubert)
(Assignee)

Updated

5 years ago
Attachment #639106 - Attachment is patch: true
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.
Attachment #639106 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 9

5 years ago
Created attachment 639121 [details] [diff] [review]
Patch with feedback/improved styling

Thanks for the feedback!
Attachment #639121 - Flags: review?(ttaubert)
(Assignee)

Updated

5 years ago
Attachment #639106 - Attachment is obsolete: true
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.
Attachment #639121 - Flags: review?(ttaubert)
(Assignee)

Comment 11

5 years ago
Created attachment 639126 [details] [diff] [review]
Added css for all themes
Attachment #639126 - Flags: review?(ttaubert)
(Assignee)

Updated

5 years ago
Attachment #639121 - Attachment is obsolete: true
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
Attachment #639126 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 639135 [details] [diff] [review]
Patch for checkin
Attachment #639126 - Attachment is obsolete: true
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
Status: NEW → ASSIGNED
Whiteboard: [good first bug][mentor=ttaubert][lang=js][lang=css] → [good first bug][mentor=ttaubert][lang=js][lang=css][fixed-in-fx-team]
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/1ec7439ba477
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=ttaubert][lang=js][lang=css][fixed-in-fx-team] → [good first bug][mentor=ttaubert][lang=js][lang=css]
You need to log in before you can comment on or make changes to this bug.