Closed Bug 631257 Opened 9 years ago Closed 9 years ago

Only default snippets are styled in Firefox Start Page design

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b12
Tracking Status
blocking2.0 --- final+

People

(Reporter: lorchard, Assigned: mak)

References

Details

(Whiteboard: [about-home][softblocker])

Attachments

(3 files, 1 obsolete file)

The new Firefox Start Page accommodates snippet content pulled from a web service (bug 592431). When the service is available, the children of div#defaultSnippets are hidden and service content is injected into div#snippetContainer

So, updates to the Start Page design (bug 627301) have two problems in this scenario:

1) Only the #defaultSnippets div has the new styling for the snippets box, so injected snippets are left unstyled.

2) The child elements of #defaultSnippets are hidden, but the empty parent div stays visible.
(In reply to comment #0)
> 1) Only the #defaultSnippets div has the new styling for the snippets box, so
> injected snippets are left unstyled.

right, the styling should be applied to both or to the container.

> 2) The child elements of #defaultSnippets are hidden, but the empty parent div
> stays visible.

right, should be collapsed.
Attached file snippet HTML from service (obsolete) —
Here's a sample of the HTML from the snippet service. It has 3 snippets, with only one revealed at random by javascript.

Haven't really styled the snippets, since I'm not sure if that styling should be a part of aboutHome.css or part of the snippet payload. So, the icons are just kind of out there
Attachment #509471 - Attachment is obsolete: true
Blocks: 630359
FWIW: I can hack around this in JS by moving the DOM node of a visible injected snippet into #defaultSnippets, but I'd like to not have to do that.
should not be hard to fix
Assignee: nobody → mak77
Status: NEW → ASSIGNED
blocking2.0: --- → ?
Blocks: 627301
Keywords: regression
OS: Mac OS X → All
Hardware: x86 → All
Status: ASSIGNED → NEW
blocking2.0: ? → ---
OS: All → Mac OS X
Hardware: All → x86
Marco - how's this fix going? We're aiming to launch this service with Beta 11.
(In reply to comment #7)
> Marco - how's this fix going? We're aiming to launch this service with Beta 11.

beta11 has already been signed off, this fix cannot make it...
Asking softblocking in the meanwhile, if we want snippets this is most likely a must-have fix.
blocking2.0: --- → ?
(In reply to comment #9)
> Asking softblocking in the meanwhile, if we want snippets this is most likely a
> must-have fix.

We do want snippets, and we're aiming to push some for beta 11. 

But, I think we can have snippets in beta 11, just with a hack-around for this design bug. The snippet code is still there in aboutHome.js, right? So, if snippets.mozilla.com goes live, beta 11 will request content?
yep, the difference is b11 is listening to http, while b12 will be on https (from tomorrow's nightly)
blocking2.0: ? → final+
Keywords: regression
Attached patch patch v1.0Splinter Review
I've simplified a bit the thing, to avoid having to manage separate styles.
"defaultSnippets" is always a hidden div with <span> default snippets in it.
"snippets" is the only styled and shown container, for both remote or local snippets.
When there are no valid remote snippets, the code takes a random <span> from "defaultSnippets" and injects it into "snippets".

Regarding the big button click, in the defaults snippets case it's easy to hook, since there is just one span with a link. In the remote snippets case, I cannot know where the link is (there is also a <script> involved), so I give you a activateSnippetsButtonClick(aElt) helper. The remote <script> will have to call it on the choosen snippet element, it doesn't matter that it's exactly inside that element (the search is recursive) but the passed-in element should have only 1 link into it.
In the example you posted above, you could just call activateSnippetsButtonClick(show_snippet);

Clearly this has still to pass the review process and names could change, but that's my basic idea, does this work for you?
Attachment #509969 - Flags: review?(gavin.sharp)
OS: Mac OS X → All
Hardware: x86 → All
(In reply to comment #12)

> Regarding the big button click, in the defaults snippets case it's easy to
> hook, since there is just one span with a link. In the remote snippets case, I
> cannot know where the link is (there is also a <script> involved), so I give
> you a activateSnippetsButtonClick(aElt) helper. The remote <script> will have
> to call it on the choosen snippet element, it doesn't matter that it's exactly
> inside that element (the search is recursive) but the passed-in element should
> have only 1 link into it.
> In the example you posted above, you could just call
> activateSnippetsButtonClick(show_snippet);
> 
> Clearly this has still to pass the review process and names could change, but
> that's my basic idea, does this work for you?

I think this is more a question for Engagement than me - this space is ultimately for the content they have planned.

Will all snippets just be one big link? If so, this might work.

But, if snippets are intended to have multiple links or even other interactive elements, then we might run into problems.
(In reply to comment #13)
> But, if snippets are intended to have multiple links or even other interactive
> elements, then we might run into problems.

It's up to your script to decide to call activateSnippetsButtonClick or not, you can avoid calling it and provide your own replacement. I don't linkify automatically the button exactly because I don't know what it contains.
and actually calling the helper on something that has more or less than 1 link is a no-op.
Whiteboard: [about-home]
(In reply to comment #13)
> (In reply to comment #12)
> I think this is more a question for Engagement than me - this space is
> ultimately for the content they have planned.
> 
> Will all snippets just be one big link? If so, this might work.
> 
> But, if snippets are intended to have multiple links or even other interactive
> elements, then we might run into problems.

From the engagement perspective our first priority is to establish feature parity to the current snippets. Those contain copy, a graphic, and one link. 

However, we would like to incorporate other enhancements like multiple links, interactive elements like a sign-up form field, and more in the future so it's important to make changes with that in mind.
Whiteboard: [about-home] → [about-home][softblock]
Mike the whiteboard [softblock] value seems bogus, platform meeting queries use the full [softblocker] string...

Btw, I think this is not that much soft, I'm not comfortable at all having the final product homepage rely on a remote hack to show snippets.
Nominating this to move from softblocker to hardblocker, since we can't really ship a home page that looks like this: 

https://bugzilla.mozilla.org/attachment.cgi?id=509469
blocking2.0: final+ → ?
Whiteboard: [about-home][softblock] → [about-home]
Thanks Alex, I was going to do the same.
From an engineering point of view I'm scared that remote snippets have to add some complicated code to workaround this bug.  It will be one of the most accessed pages.
(In reply to comment #19)
> From an engineering point of view I'm scared that remote snippets have to add
> some complicated code to workaround this bug.  It will be one of the most
> accessed pages.

Agree that a workaround isn't desirable. As we're aiming for FxB 12 to launch this service can you give us an idea of what needs to be done so that a workaround won't be needed?
(In reply to comment #20)
> can you give us an idea of what needs to be done so that a
> workaround won't be needed?

Review and land the patch in this bug.
This remains a soft blocker for final; if we don't get it for Firefox 4, then we can do a workaround on the server side or get the right fix out there in the first maintenance release.

I would not hold the product on this bug. That said, I also believe that it should be a high priority as a soft blocker. Just waiting on Gavin's review.
blocking2.0: ? → final+
Whiteboard: [about-home] → [about-home][softblock][needs review Gavin]
Attachment #509969 - Flags: review?(gavin.sharp) → review+
Whiteboard: [about-home][softblock][needs review Gavin] → [about-home][softblocker][needs review Gavin]
Keywords: checkin-needed
Whiteboard: [about-home][softblocker][needs review Gavin] → [about-home][softblocker][can land]
http://hg.mozilla.org/mozilla-central/rev/346e58464d3d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [about-home][softblocker][can land] → [about-home][softblocker]
Target Milestone: --- → Firefox 4.0b12
msucan reported and I can confirm that the button-link-logic here seems to be going awry; can't tell if that's because of the way the snippet is being served up, or the way we're trying to detect the links, but the buttons generated by the  snippets that we're getting from the server look like you can click the entire button, but only the link goes somewhere.

Followup?
that's bug 634504, should start working from today's update.
Verified using Mozilla/5.0 (Windows NT 6.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.