Closed Bug 974745 Opened 7 years ago Closed 7 years ago

Create click event on Sponsored Tiles to show explanation panel

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31

People

(Reporter: jboriss, Assigned: mzhilyaev)

References

()

Details

(Whiteboard: [tiles] p=8 s=it-31c-30a-29b.2 [qa-])

Attachments

(2 files, 6 obsolete files)

This hover event spawns from Sponsored Tile icons (added in bug 974736).  The panel created briefly explains what Sponsored Tiles are and links to a Mozilla Public Page providing more information (page created in bug 974728).

Future version contain have additional information and features on this panel, but this basic first version only gives text a link.

Acceptance criteria: 

1. On mouseover with standard tooltip delay, the a Sponsored Tile panel is displayed
2. On cessation of mouseover, the panel fades quickly and disappears
3. On click, the panel displays and maintains visibility even if the user ceases mouseover.  Panel is only dismissed from this state if user clicks else (in New Tab page, on the icon itself, or in Firefox chrome)
4. On Mozilla Public Page link click, the Mozilla Public Page explaining Sponsored Tiles opens in the current tab.
Depends on: 974736
What should happen if there is not enough screen space at the bottom to show the panel? The mockup has the panel with an arrow pointing at the sponsored icon (similar to the doorhanger in Firefox). I see that the doorhanger does tries to keep the arrow pointing upwards/downwards towards the left/right side of the panel as it can fit. We should mimic that behavior?
Flags: needinfo?(jboriss)
(In reply to Edward Lee :Mardak from comment #1)
> What should happen if there is not enough screen space at the bottom to show
> the panel? The mockup has the panel with an arrow pointing at the sponsored
> icon (similar to the doorhanger in Firefox). I see that the doorhanger does
> tries to keep the arrow pointing upwards/downwards towards the left/right
> side of the panel as it can fit. We should mimic that behavior?

Exactly right - it should mimic the behavior of the doorhanger, which in this case would mainly appear upwards of the icon.
Flags: needinfo?(jboriss)
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #0)
> 2. On cessation of mouseover, the panel fades quickly and disappears
So just to be clear, even if the user tries to click the "Learn more..." link, it should fade unless the user clicked the icon to make it persist? Or should there be a delay so that if the user moves into the panel within that time, the panel will persist?
Flags: needinfo?(jboriss)
We might be running into bug 962124 because the arrow panel transitions in (shifting down) as it appears, so that's why the mouse enters the panel when it first appears.
Bug 956162 recently landed implementing @flip=none which should prevent the animation but arrowpanels are hardcoded to @flip=both. Although I'm not sure if we actually want @flip=none because it'll prevent the logic from attempting to keep the panel visible on screen.
As noted in bug 974736#c29, this is changing from a hover event to a click event.

(In reply to Ed Lee :Mardak from comment #4)
> (In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #0)
> > 2. On cessation of mouseover, the panel fades quickly and disappears
> So just to be clear, even if the user tries to click the "Learn more..."
> link, it should fade unless the user clicked the icon to make it persist? Or
> should there be a delay so that if the user moves into the panel within that
> time, the panel will persist?

With this as a click event, the panel should fade if the user clicks anywhere outside of the panel.  Panel will be persistent until next click and is activated via icon click.
Flags: needinfo?(jboriss)
Summary: Create hover event on Sponsored Tiles to show explanation panel on mouseover (with delay) → Create click event on Sponsored Tiles to show explanation panel on mouseover (with delay)
Blocks: tiles-dev
Whiteboard: [tiles] → [tiles] p=0
Assignee: nobody → mzhilyaev
Attached patch v1 (obsolete) — Splinter Review
adw, a couple questions regarding this patch:

1) should the panel's xul:label use value=&text instead of >&text</
2) should we add a new sponsoredPanel.js that knows how to show instead of having it part of Site as Site_showSponsoredPanel
Attachment #8386893 - Flags: review?(adw)
Attached patch v2 (obsolete) — Splinter Review
Simplified the panel structure and fixed styling per Boriss' comments
Attachment #8386893 - Attachment is obsolete: true
Attachment #8386893 - Flags: review?(adw)
Attachment #8387440 - Flags: review?(adw)
Comment on attachment 8387440 [details] [diff] [review]
v2

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

Looks OK to me, except I think it'd be better to define showSponsoredPanel on gPage in page.js instead of on Site.prototype, since the panel is a property of the page, not each site, even though the panel pops up on sites.  Site.prototype._onClick would still handle the click but pass it off to gPage.showSponsoredPanel.  Sound OK?

::: browser/base/content/newtab/sites.js
@@ +226,5 @@
>  
>    /**
> +   * Opens sponsored button panel
> +   */
> +  showSponsoredPanel: function Site_showSponsoredPanel(target) {

Nit: aTarget to match the style in the newtab code.

@@ +229,5 @@
> +   */
> +  showSponsoredPanel: function Site_showSponsoredPanel(target) {
> +    if (this._sponsoredPanel.state == "closed") {
> +      this._sponsoredPanel.addEventListener("popuphidden", function onPopupHidden(e) {
> +        this._sponsoredPanel.removeEventListener("popuphidden", onPopupHidden, false);

This doesn't actually remove the listener, because the listener is the function returned by bind(), not onPopupHidden, right?
Attachment #8387440 - Flags: review?(adw) → feedback+
(In reply to Ed Lee :Mardak from comment #8)
> Created attachment 8386893 [details] [diff] [review]
> v1
> 
> adw, a couple questions regarding this patch:
> 
> 1) should the panel's xul:label use value=&text instead of >&text</
> 2) should we add a new sponsoredPanel.js that knows how to show instead of
> having it part of Site as Site_showSponsoredPanel

Missed these.

1) The value attribute is fine.
2) Think I inadvertently addressed this.  I don't think a new file is necessary.
(In reply to Drew Willcoxon :adw from comment #10)
> Looks OK to me, except I think it'd be better to define showSponsoredPanel
> on gPage in page.js instead of on Site.prototype, since the panel is a
> property of the page, not each site, even though the panel pops up on sites.
> Site.prototype._onClick would still handle the click but pass it off to
> gPage.showSponsoredPanel.  Sound OK?
Sounds great. Thanks.

> > +      this._sponsoredPanel.addEventListener("popuphidden", function onPopupHidden(e) {
> > +        this._sponsoredPanel.removeEventListener("popuphidden", onPopupHidden, false);
> This doesn't actually remove the listener, because the listener is the
> function returned by bind(), not onPopupHidden, right?
Ah nice catch. I explicitly said not to use => because arguments.callee wouldn't work correctly for remove. I believe the pattern is to just assign the bound handler and use that for both add/remove. Or do the let self = this and use anonymous/arguments.callee.
Attached patch v3 (obsolete) — Splinter Review
Moved showSponsoredPanel logic into page.js
Attachment #8387440 - Attachment is obsolete: true
Attachment #8389383 - Flags: review?(adw)
Attachment #8389383 - Attachment description: bug974745.patch → v3
Attached patch V3.1 (obsolete) — Splinter Review
Clarified css comment.
Attachment #8389383 - Attachment is obsolete: true
Attachment #8389383 - Flags: review?(adw)
Attachment #8389433 - Flags: review?(adw)
Comment on attachment 8389433 [details] [diff] [review]
V3.1

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

::: browser/base/content/newtab/sites.js
@@ +213,5 @@
>  
>      aEvent.preventDefault();
> +    if (target.classList.contains("newtab-control-sponsored"))
> +      gPage.showSponsoredPanel(target);
> +    else if (target.classList.contains("newtab-control-block"))

Nit: Would you mind adding your new conditional after the existing if, as an else if?  That way the patch doesn't change more lines than necessary, and blame is preserved for the existing if.
Attachment #8389433 - Flags: review?(adw) → review+
Attached patch for checkin (obsolete) — Splinter Review
Attachment #8389433 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Whiteboard: [tiles] p=0 → [tiles] p=3 s=it-31c-30a-29b.1
I think this should be covered by automation.
Flags: in-testsuite?
QA Contact: mihaela.velimiroviciu
Whiteboard: [tiles] p=3 s=it-31c-30a-29b.1 → [tiles] p=3 s=it-31c-30a-29b.1 [qa-]
maksik: we'll want a unit test for synthesizing a mouse event on the icon and checking if the panel appears then disappears when clicking away
Summary: Create click event on Sponsored Tiles to show explanation panel on mouseover (with delay) → Create click event on Sponsored Tiles to show explanation panel
Attached patch for checkin (obsolete) — Splinter Review
Adds test and emtwo's fix for styling the sponsored icon as 14x14 instead of 16x16.
Attachment #8391048 - Attachment is obsolete: true
maksik, the test seems to only work on Mac and is failing on Linux and Windows. I'm guessing it's related to how you generate mouse down/up events.
Attached patch for checkinSplinter Review
Avoids Linux and Windows test failures by closing the popup and checking for correct states instead of relying on clicks to dismiss.
Attachment #8398868 - Attachment is obsolete: true
The test failing: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-content-events.js#81

TEST-UNEXPECTED-FAIL | tests/test-content-events.test nested frames | events where dispatched - [
    "document-element-inserted -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "DOMContentLoaded -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "document-element-inserted -> data:text/html,iframe",
    "DOMContentLoaded -> data:text/html,iframe",
    "load -> data:text/html,iframe",
    "pageshow -> data:text/html,iframe",
    "load -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "pageshow -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E"
] deepEqual [
    "document-element-inserted -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "DOMContentLoaded -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "document-element-inserted -> chrome://global/skin/arrow/panelarrow-vertical.svg",
    "document-element-inserted -> data:text/html,iframe",
    "DOMContentLoaded -> data:text/html,iframe",
    "load -> data:text/html,iframe",
    "pageshow -> data:text/html,iframe",
    "load -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E",
    "pageshow -> data:text/html,%3Ciframe%20src='data:text/html,iframe'%3E"
]

It sees a document-element-inserted with panelarrow-vertical.svg... on some runs. I'm guessing maybe it wasn't fully loaded from a previous test that did open a new tab page?
The test seems to also fail sometimes for this one: http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-content-events.js#39

TEST-UNEXPECTED-FAIL | tests/test-content-events.test multiple tabs | all events dispatche as expeced - [
    "document-element-inserted -> data:text/html,first-tab",
    "DOMContentLoaded -> data:text/html,first-tab",
    "load -> data:text/html,first-tab",
    "pageshow -> data:text/html,first-tab",
    "document-element-inserted -> data:text/html,second-tab",
    "DOMContentLoaded -> data:text/html,second-tab",
    "load -> data:text/html,second-tab",
    "pageshow -> data:text/html,second-tab"
] deepEqual [
    "document-element-inserted -> chrome://global/skin/arrow/panelarrow-vertical.svg",
    "document-element-inserted -> data:text/html,first-tab",
    "DOMContentLoaded -> data:text/html,first-tab",
    "load -> data:text/html,first-tab",
    "pageshow -> data:text/html,first-tab",
    "document-element-inserted -> data:text/html,second-tab",
    "DOMContentLoaded -> data:text/html,second-tab",
    "load -> data:text/html,second-tab",
    "pageshow -> data:text/html,second-tab"
]
Depends on: 990284
Whiteboard: [tiles] p=3 s=it-31c-30a-29b.1 [qa-] → [tiles] p=3 s=it-31c-30a-29b.2 [qa-]
https://hg.mozilla.org/mozilla-central/rev/f3c44cfc3900
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Status: RESOLVED → VERIFIED
Whiteboard: [tiles] p=3 s=it-31c-30a-29b.2 [qa-] → [tiles] p=8 s=it-31c-30a-29b.2 [qa-]
Depends on: 990977
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Blocks: 991542
Component: Tabbed Browser → General
Blocks: 995436
Blocks: 1040369
You need to log in before you can comment on or make changes to this bug.