Closed Bug 1145303 Opened 5 years ago Closed 4 years ago

Remove unused "What is this page?" link and related intro messaging

Categories

(Firefox :: New Tab Page, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: Mardak, Assigned: ursula)

References

Details

(Whiteboard: .?)

Attachments

(1 file, 7 obsolete files)

To keep the functionality of showing the new tab notice for first users but moving it out of the way, we'll try moving the link/anchor to the bottom middle.
Assignee: nobody → msamuel
Attached image bottom.png (obsolete) —
How does this look Aaron?
Attachment #8582601 - Flags: ui-review?(athornburgh)
This looks great! 

Could you maybe move the text up 5 or 10 pixels so that there's a bit more padding at the bottom?

Otherwise perfect. Thank you.
Attachment #8582601 - Flags: ui-review?(athornburgh)
emtwo, how does the panel behave if the screen is maximized vs small enough where potentially the panel opens below the Firefox window?
Attached image smallscreen_before.png (obsolete) —
Attached image smallscreen_after.png (obsolete) —
Attached image reasonable.png (obsolete) —
Ed, the smallscreen_before.png and smallscreen_after.png show the difference in the panel with this patch. It does look slightly uglier with this change, although in both cases, it is chopped off.

I also added a screenshot with how it looks with one row of tiles. The arrow at the bottom of the panel seems to disappear but the full text is still visible. Though, reducing the width of the window causes "suggested for hrblock.com visitors" to overlap with "what is this page", so I will need to fix that.

Perhaps we need to decide how we want to handle the chopped off text given the way the arrow panel functions?

It looks like the previous implementation wouldn't open the panel at all if the browser window was under a certain width.
Comment on attachment 8582624 [details] [diff] [review]
v1: Move "What is this page?" to the bottom of newtab

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

This makes the click target the entire width of the page, so I don't really want to r+ this even though it's nice and simple.  Could you please try something else so the click target is only the text?  Maybe it would be simplest to not absolutely position it but put it below the grid in the markup -- actually it's already below the grid in the markup.  You'd have to update the availSpace calculation though.
Attachment #8582624 - Flags: review?(adw)
* No longer has the full page width as a target
* sets min width and height for the explanation text so it does not cut off like in the screenshots
Attachment #8582624 - Attachment is obsolete: true
Attachment #8587518 - Flags: review?(adw)
Comment on attachment 8587518 [details] [diff] [review]
v2: Move "What is this page?" to the bottom of newtab

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

A couple of things: The `width: 110px` isn't really good for localization since the string will be different lengths in different languages.  And if you change the window's height, you can sometimes see the vertical scrollbar appear and the What's new is pushed off-screen.

To fix the first problem, I tried using an inline element -- a span instead of a div -- and that worked OK:

    <div id="what-wrapper">
      <span id="newtab-intro-what">&newtab.customize.what;</span>
    </div>

#newtab-intro-what {
  cursor: pointer;
}

#what-wrapper {
   position: relative;
   text-align: center;
   margin: 25px;
}

For the second problem, you need to change the availSpace calculation in grid.js to this or something like it:

    let whatWrapper = document.querySelector("#what-wrapper");
    let availSpace = document.documentElement.clientHeight -
                     this._cellMargin -
                     document.querySelector("#newtab-search-container").offsetHeight -
                     whatWrapper.offsetHeight -
                     parseFloat(getComputedStyle(whatWrapper).marginTop) -
                     parseFloat(getComputedStyle(whatWrapper).marginBottom);

Whatever you use, please make sure that the scrollbar doesn't appear when you resize the window.

And a couple of nits: Could you please rename what-wrapper to newtab-intro-what-container to be more consistent with the page's naming scheme?  (Personally I don't think the newtab- prefix is necessary, but it's there, so let's be consistent.)
Attachment #8587518 - Flags: review?(adw)
(In reply to Drew Willcoxon :adw from comment #11)
> appear and the What's new is pushed off-screen.

Er, What is this page, I mean.
Sorry for the churn here.

dcrobot, if we aren't moving the "whats this page" to the bottom in firefox 38 (turns out to be trickier than just a simple move), and your new designs for firefox 39 in bug 1150228 doesn't have a "what is this page?" either as a link or a menu item, then this bug should be more of remove "what is this page" and related intro popup?
Flags: needinfo?(athornburgh)
Perhaps. We actually need to determine a longer term strategy that addresses:

1.) legal requirements

2.) international requirements

3.) show users things in content - as they do stuff - instead of "show" them everything up front

4.) minor feature updates vs. updates to the larger experience


For now, I believe that everyone agreed "What is this page?" would no longer be necessary for 39 since we're introducing the on-boarding experience (which explains all the key points made in the existing doorhanger).

-A
Flags: needinfo?(athornburgh)
Per comment 14, morphing this bug and deprioritizing for now as we'll want the new onboarding experience before removing the legal intro messaging.

We'll probably end up reusing some of the firstrun/intro-shown logic to trigger the onboarding experience.
Assignee: msamuel → nobody
Depends on: 1138818
Summary: Move "What is this page?" to the bottom middle → Remove "What is this page?" link and related intro messaging
Blocks: 1155443
No longer blocks: 1155443
No longer depends on: 1138818
Depends on: 1138818
Blocks: 1140185
No longer blocks: 1120311
Patrick, we are removing the 'What is this page' link on the new tab and moving it under 'Learn about New Tab' under the cog wheel menu options. That option will link to the existing https://www.mozilla.org/en-US/firefox/tiles/ 

'Learn about New Tab' will also be on the bottom of the onboarding tutorial as well as the 'About your privacy' link which goes to https://www.mozilla.org/en-US/privacy/

Within the Suggested Tiles in-tile disclaimer, activated by clicking on the Suggested label, will link to the SUMO page, https://support.mozilla.org/en-US/kb/how-do-tiles-work-firefox
Flags: needinfo?(pfinch)
Depends on: 1150180
Flags: needinfo?(pfinch)
(In reply to Kevin Ghim from comment #16)
> Patrick, we are removing the 'What is this page' link on the new tab and
> moving it under 'Learn about New Tab' under the cog wheel menu options. That
> option will link to the existing
> https://www.mozilla.org/en-US/firefox/tiles/ 

Created dependency on updating that page.  
 
> Within the Suggested Tiles in-tile disclaimer, activated by clicking on the
> Suggested label, will link to the SUMO page,
> https://support.mozilla.org/en-US/kb/how-do-tiles-work-firefox

That work is underway, adding Joni Savage.
For 40, we'll remove the link for all locales. For 39 uplift, we want the link removed for en-US.
The onboarding patch bug 1138818 already hides the intro link and stuff, so this bug is now only to remove this unused code. Not critical for 39.
Blocks: 1145418
No longer blocks: 1140185
Summary: Remove "What is this page?" link and related intro messaging → Remove unused "What is this page?" link and related intro messaging
Duplicate of this bug: 1167610
Assignee: nobody → ursulasarracini
Attachment #8609586 - Flags: review?(mconley)
Attachment #8609586 - Flags: review?(msamuel)
Comment on attachment 8609586 [details] [diff] [review]
Remove unused 'What is this page?' link and related intro messaging

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

Some leftovers:

There are still styles for newtab-intro-panel in browser/themes/linux/newtab/newTab.css, browser/themes/osx/newtab/newTab.css and browser/themes/windows/newtab/newTab.css, according to DXR.

Still a style for newtab-intro-what in browser/themes/shared/newtab/newTab.inc.css

Note that I haven't been in about:newtab code in a while, and that :emtwo or :Mardak's comments, if they contract me, should override me, since they probably know far more about what's going on in here. :)

::: browser/base/content/newtab/newTab.css
@@ +54,1 @@
>    display: none;

I think :emtwo should probably weigh in here, but I think this isn't exactly what we want to do.

The old rule:

#newtab-scrollbox[page-disabled] #newtab-intro-what {	
  display: none;	
}

was hiding the #newtab-intro-what if #newtab-scrollbox had the page-disabled attribute.

This change makes it so that we're hiding the #newtab-scrollbox if it has page-disabled on it.

I have a feeling we don't want that. This entire rule can probably go.

::: browser/base/content/newtab/newTab.xul
@@ -25,5 @@
>    <div id="newtab-customize-overlay"></div>
>  
> -  <xul:panel id="newtab-intro-panel" orient="vertical" type="arrow"
> -             noautohide="true" hidden="true">
> -    <h1>&newtab.intro.header;</h1>

I don't believe this string is being used anymore, so it can probably be removed from the newTab.dtd file.

@@ -131,5 @@
>        <div id="newtab-margin-bottom"/>
>  
>      </div>
>  
> -    <div id="newtab-intro-what">&newtab.customize.what;</div>

I think there's a test that checks whether or not this element exists that is going to fail with this patch:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/newtab/browser_newtab_enhanced.js#112

We should update that test as well, and make sure it still passes.
Attachment #8609586 - Flags: review?(mconley) → review-
> ::: browser/base/content/newtab/newTab.css
> @@ +54,1 @@
> >    display: none;
> 
> I think :emtwo should probably weigh in here, but I think this isn't exactly
> what we want to do.

that's right, the whole rule can go.
Attachment #8609586 - Attachment is obsolete: true
Attachment #8609586 - Flags: review?(msamuel)
Attachment #8609605 - Flags: review?(msamuel)
Attachment #8609605 - Flags: review?(mconley)
Comment on attachment 8609605 [details] [diff] [review]
Remove unused 'What is this page?' link and related intro messaging

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

This looks good to me! Let's wait for emtwo to give feedback too, though.

In the meantime, I'll push to try for you.
Attachment #8609605 - Flags: review?(mconley) → review+
Comment on attachment 8609605 [details] [diff] [review]
Remove unused 'What is this page?' link and related intro messaging

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

This looks good to me, thank you!
Attachment #8609605 - Flags: review?(msamuel) → feedback+
Attachment #8587518 - Attachment is obsolete: true
Attachment #8583236 - Attachment is obsolete: true
Attachment #8582601 - Attachment is obsolete: true
Attachment #8583234 - Attachment is obsolete: true
Attachment #8583235 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/00845450a0b6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
I don't think this is necessary for uplifting to 40.
No longer blocks: 1145418
You need to log in before you can comment on or make changes to this bug.