Closed Bug 1143797 Opened 5 years ago Closed 5 years ago

Allow clicking on suggested explanation text to see overlay explaining the suggested tile

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: Mardak, Assigned: emtwo)

References

Details

(Whiteboard: .003)

Attachments

(7 files)

Similar to bug 1138817, but instead of triggering from [suggested], the user clicks on the explanation label.

dcrobot has ideas of on hover styling to let the user know it's clickable, e.g., pointer cursor, outline/shadow of a raised button.
Text: This site is suggested to you by Firefox. You can remove it at any time by clicking the [X] button. Learn more…

So probably..

This site is suggested to you by Firefox. You can remove it at any time by clicking the %1$S button. %2$S
Assignee: nobody → msamuel
Sorry, another string change:

"This site is suggested to you by Firefox. You can remove it at any time by clicking the %1$S button. %2$S"

to:

"This site is suggested to you by Mozilla. You can remove it at any time by clicking the %1$S button. %2$S"
Iteration: --- → 39.2 - 23 Mar
Flags: firefox-backlog+
Whiteboard: .? → .003
Status: NEW → ASSIGNED
Flags: qe-verify?
Note: I also made several relevant changes from 'related' -> 'suggested' since we're moving toward renaming all the variables (bug 1136208)
Attachment #8580941 - Flags: review?(adw)
Attached image click_state.png
Attached image hover_state.png
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Comment on attachment 8580941 [details] [diff] [review]
v1: Add suggestion legal text explanation overlay on user text click

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

Sorry for the delay, Marina.

This is unrelated to your patch, but I notice that the X button in the legal text overlay isn't vertically centered with the text in its line.  Would you mind filing a bug to fix that?

::: browser/base/content/newtab/sites.js
@@ +242,5 @@
>        button.removeAttribute("active");
>      }
>      else {
>        let explain = document.createElementNS(HTML_NAMESPACE, "div");
> +      explain.className = explanationTextClass.slice(1);

Please add a brief comment that the slice is to remove the dot.
Attachment #8580941 - Flags: review?(adw) → review+
Blocks: 1141617
Attached image legal_misaligned.png
Drew, just confirming I understood correctly, do you mean the (X) should be vertically centered instead of having its bottom aligned with the text there? Thanks!
Flags: needinfo?(adw)
Yeah, I think so anyway. :-)  I don't mean for that to block any of your ongoing work though.
Flags: needinfo?(adw)
Oh, it's not blocking. I was just wondering because the bottom aligned seemed like what I would expect :) Either way, now it's filed!
https://hg.mozilla.org/mozilla-central/rev/e70798a79585
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Blocks: 1148859
Flags: qe-verify? → qe-verify+
QA Contact: cornel.ionce
Points: --- → 5
Verified fixed on Windows 7 64-bit, Mac OS X 10.9.5, Windows 8.1 64-bit and Ubuntu 12.04 32-bit using:
- Firefox Aurora (build ID: 20150405004004);
- Latest Nightly (build ID: 20150405030238)
Status: RESOLVED → VERIFIED
Also verified with Firefox 38 beta 2, build ID: 20150406174117 on Windows 7 64-bit, Mac OS X 10.9.5, and Ubuntu 14.04 32-bit.
Blocks: 1155443
No longer blocks: 1155443
It looks like this was the beta (Fx38) uplift patch for this bug: 

https://hg.mozilla.org/releases/mozilla-beta/rev/65f2aa5f2dd7
You need to log in before you can comment on or make changes to this bug.