Closed Bug 1068181 Opened 5 years ago Closed 5 years ago

NEW Indicator for Pinned Tiles on New Tab Page

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.2
Tracking Status
firefox33 + verified
firefox34 --- verified
firefox35 --- verified

People

(Reporter: athornburgh, Assigned: Mardak)

References

Details

Attachments

(7 files, 2 obsolete files)

Attached image 01 - Initial View.png
Problem: The bolded, blue titles beneath the tiles does not clearly indicate "pinned" and introduces confusion.

Solution: For pinned tiles, SHOW AN ICON instead of changing the styling of the tile label.

Attached is a comp and the image assets.

Please place the icon beneath the title – 8 pixels below the bottom edge of the tile – and aligned LEFT.

This change is approved by the FX Desktop Team. Is it possible to make this change in time for our initial release?
Attached image Icon_Pinned_10x17.svg
This is the SVG icon to indicate pinned tiles.
What should happen if the title is long? Is this in addition to the blue pinned indicator on hover? Compared to what's currently in Release, the description in comment 0 seems to be that the bold/blue indicator caused this confusion, so why not revert to what it was before?
Component: General → New Tab Page
OS: Mac OS X → All
QA Contact: philipp
Hardware: x86 → All
(In reply to Ed Lee :Mardak from comment #2)
> What should happen if the title is long? Is this in addition to the blue
> pinned indicator on hover? Compared to what's currently in Release, the
> description in comment 0 seems to be that the bold/blue indicator caused
> this confusion, so why not revert to what it was before?

I can't find an example right now, but I'm assuming we already have a way of dealing with collisions between the titles and the [Sponsored] indicator. We can shrink the size of the text area in general to make room for the pin.

And yes, this is in addition to the blue pin :)

Some kind visible indication for pinned tabs makes sense because especially with enhanced tiles, the image of tiles might change quite a bit over time. Showing the pin is a good way to reassure the user that this is still the same tile he pinned earlier, without getting in their way too much.
There's no explicit handling of [sponsored] and long titles as the title shown is controlled and should short. But pinned tiles can be for any tile so history titles could be long. I'm assuming if we do shorten the title so that it doesn't overlap the pinned indicator, the title should still appear visually centered with the tile? Should the title extend all the way to the right edge or crop sooner so it's balanced?

Centered ignoring the [p] pin indicator space:
[tile image goes here.]
[p]     center

Longer title extending all the way right:
[tile image goes here.]
[p] title title titl...

Longer title equally balanced center (as if there was an invisible [p] on the right):
[tile image goes here.]
[p] center title...
Yeah, it should be cut off equally to look centered at all times (so option 3).

Perhaps we can move it a few pixels to the right to balance out the »...« which have less visual weight.
But that last part is fine-tuning and could totally be a follow-up

Aaron, please chime in here if you disagree with any of that :)
I agree! This pin is only 10 pixels wide... add 5 pixels for padding, and you only lose 15 pixels altogether.
(In reply to Aaron from comment #7)
> I agree! This pin is only 10 pixels wide... add 5 pixels for padding, and
> you only lose 15 pixels altogether.

Well, it's really 30 pixels since it's on both sides, right?
Should still be unproblematic though.
Who would know how to convert attachment 8490237 [details] svg to be like http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/newtab/controls.svg?raw=1 ?

The attached file seems to have a shadow encoded in base64. I'm guessing the <path> would be similar to the existing     <path    id="glyphShape-pin" d="M19,15v-5h2V7H11v3h2v5l-3,2v2h5c0,4.5,0.4,8,1,8s1-3.5,1-8h5v-2L19,15z" /> if not the same? There just wouldn't be a #glyphShape-circle for the icon. ?
Attached patch svg diff (obsolete) — Splinter Review
Here's my attempt at editing the svg file directly. I've never done this before, so danger! :p
Attached image controls.svg
Resulting controls.svg from attachment 8490339 [details] [diff] [review] patch.
Hey Ed, is it possible to implement and uplift this to beta before tiles move to Release in ~3 weeks?
Flags: needinfo?(edilee)
You can set the various backlog and tracking flags (e.g., tracking-firefox33) with explanations of why this needs to be uplifted.

In terms of implementation, I would expect the changes to be css, svg, xul changes; so no js code to review. Should be relatively quick to fix when prioritized.
Points: --- → 3
Flags: needinfo?(edilee)
[Tracking Requested - why for this release]: Tiles will hit GA with the 33 train. The current way of indicating pinned tiles (blue and bold text) is ambiguous and has caused some confusion. In order not to carry that confusion to GA, we need to move this to beta before the trains.
OK. If possible, I would like that to land before beta 8 (September, 29th).
Attached patch v1 (obsolete) — Splinter Review
Attachment #8490339 - Attachment is obsolete: true
Attachment #8494993 - Flags: review?(adw)
Attached image v1 screenshot
So why are we using an SVG here? Shouldn't we just export that to PNG in 1x and 2x resolution like we do for every other icon?
Flags: needinfo?(edilee)
Oh, we're already using an SVG for the controls, I see. Nevermind then, didn't know we changed that.
Flags: needinfo?(edilee)
Comment on attachment 8494993 [details] [diff] [review]
v1

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

::: browser/themes/shared/newtab/newTab.inc.css
@@ +146,5 @@
>  }
>  
>  .newtab-site[pinned] .newtab-title {
> +  padding: 0 15px;
> +}

This leads to an unnecessary padding on the right with long titles as seen in your screenshot. We can remove this if below we remove |position: absolute|, |left: 0|, and insert:

float: left;
-moz-margin-end: 5px;

And don't forget RTL :)

-moz-locale-dir(rtl) {
  float: right;
}
Attachment #8494993 - Flags: review?(adw) → feedback+
(In reply to Tim Taubert [:ttaubert] from comment #20)
> This leads to an unnecessary padding on the right with long titles as seen
> in your screenshot.
Thanks. I was thinking about that as well, but comment 6 says to keep the title centered at all times although it also mentions perhaps offsetting it a little because the ... isn't as heavy as text.

The tricky aspect of putting margins on one side is that the text might not be perfectly centered with the tile (even without the ...).
Attached image float screenshot
(In reply to Tim Taubert [:ttaubert] from comment #20)
> float: left;
> -moz-margin-end: 5px;
Here's a screenshot where the title is no longer centered with the tile. Pinning and unpinning the tile causes the title to jump right/left a little bit.
You're right, that is worse than losing 15px on the right I guess. Too bad, sorry. The only thing left to fix is then doing {left: auto; right: 0} for RTL builds.
Attached patch v2Splinter Review
Attachment #8494993 - Attachment is obsolete: true
Attachment #8495055 - Flags: review?(ttaubert)
Comment on attachment 8495055 [details] [diff] [review]
v2

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

Thanks!
Attachment #8495055 - Flags: review?(ttaubert) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/a4c45db3f0a6
Flags: firefox-backlog+
Target Milestone: --- → Firefox 35
https://hg.mozilla.org/mozilla-central/rev/a4c45db3f0a6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Iteration: --- → 35.2
Flags: qe-verify?
Ed, I am sure you are going to ask for an uplift in aurora & beta but n-i to make sure this happens asap. Thanks
Flags: needinfo?(edilee)
Attached patch for upliftSplinter Review
Flags: needinfo?(edilee)
Comment on attachment 8496362 [details] [diff] [review]
for uplift

Approval Request Comment
[Feature/regressing bug #]: Bug 1030832 / Enhanced Tiles
[User impact if declined]: Confusion about why titles are blue and bold
[Describe test coverage new/current, TBPL]: None
[Risks and why]: Low - style tweaks to css and svg files
[String/UUID change made/needed]: None
Attachment #8496362 - Flags: approval-mozilla-beta?
Attachment #8496362 - Flags: approval-mozilla-aurora?
Attachment #8496362 - Flags: approval-mozilla-beta?
Attachment #8496362 - Flags: approval-mozilla-beta+
Attachment #8496362 - Flags: approval-mozilla-aurora?
Attachment #8496362 - Flags: approval-mozilla-aurora+
QA Contact: cornel.ionce
Verified fixed on Windows 7 64-bit, Windows 8.1 64-bit, Ubuntu 12.04 and Mac OS X 10.9 using:
- latest Nightly, build ID: 20140928030206
- latest Aurora, build ID: 20140928004003
Confirming this fix on Firefox 33 beta 8, build ID: 20140929180120.

User Agents:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
Status: RESOLVED → VERIFIED
Flags: qe-verify? → qe-verify+
You need to log in before you can comment on or make changes to this bug.