1168955 - [User Story] Preview Pin Badge

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Ben Francis [:benfrancis]]

As a user I want to preview the pin badge of a web site so I can see what it will look like if I pin it.

Comment 1

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/95560434

Comment 2

[Alberto Pastor [:albertopq]]

Hey Eric, do you want to take a look before I start working? It would be great to have some kind of quick guide like the one you did for the Pin Page. Thanks!

Comment 3

[Eric Pang [:epang] UX]

(In reply to Alberto Pastor [:albertopq] from comment #2)
> Hey Eric, do you want to take a look before I start working? It would be
> great to have some kind of quick guide like the one you did for the Pin
> Page. Thanks!

Is there any documentation/flows for what should be shown?  I'm not clear on the user story. Thanks!

Comment 4

[Alberto Pastor [:albertopq]]

Hey Eric,

You can find it here: https://docs.google.com/presentation/d/17CGWPwu59GB7miyY1ErTjr4Wb-kS-rM7dB3MAMVO9HU/pub?start=false&loop=false&delayms=3000#slide=id.g9cce20abd_2_36

Thanks!

Comment 5

[Alberto Pastor [:albertopq]]

Hey Eric, was the info I linked in the previous comment enough? Let me know if I can help on anything else.
Thanks!

Comment 6

[Ben Francis [:benfrancis]]

Attachment: Preview Pin Badge - Specification

Comment 7

[Ben Francis [:benfrancis]]

I've attached the baseline spec, the only thing that is different for this user story than the preview card one which Eric provided visuals for is that you should see a 64x64px icon (when at 1x pixel density) and the site name (which I have at 1.4rem Fira Sans) underneath instead of the card. Switching between pinning a page and pinning the site is covered by bug 1168958 but you may want to do that all in one patch.

I think Eric wants to tweak the close and arrow buttons to re-use existing components and he may want to change the font style, but that shouldn't be more than asset swaps which could be done in a follow-up, so I wouldn't block on visuals.

Comment 8

[Alberto Pastor [:albertopq]]

Sorry, I won't have time to work on this until next week. Unassigned, just in case somebody else can take a look this week.

Comment 9

[Eric Pang [:epang] UX]

Hi, I've uploaded a visual spec here:
https://drive.google.com/open?id=0B9o41Fwl8FagSXRNYUozb0NmWGM

Image assets can be found here:
https://drive.google.com/open?id=0B9o41Fwl8FagflI5bElUUERjYXNERUpTckhWV2Y1QjVBdDE5cTJyTUFrM0ljWkdrcXVKUG8

This spec also updates the visual design of the preview card overall.  If needed I can open a new bug for anything that doesn't pertain to the user story of this bug. :)

Thanks!

Comment 10

[Ben Francis [:benfrancis]]

(In reply to Eric Pang [:epang] from comment #9)
> Hi, I've uploaded a visual spec here:
> https://drive.google.com/open?id=0B9o41Fwl8FagSXRNYUozb0NmWGM

Thanks Eric, this looks good. Heads up that changes to the interaction design may be incoming, but hopefully a lot of your work is re-usable https://docs.google.com/presentation/d/1Xja64Radti_xz-WMoI1hmixkXPSTb4xjdxnr-PvzHhM/pub?start=false&loop=false&delayms=3000

The one thing I'm not sure about with your visual spec is the "scrim" (as Francis calls them) which obscures content and makes the doorhanger a lot like a modal dialog. We're considering using the door hanger for informational prompts which the user may want to ignore so I'm a bit concerned that obscuring the whole screen might be a bit jarring?

As a data point, the door hangers in Firefox and Firefox for Android don't use a translucent scrim. They do allow you to click outside the door hanger to dismiss it, but the scrim is completely transparent.

Also, the subtle low-contrast distinction between button and background colour reminds me that I really need to replace my monitors! :P

Comment 11

[Eric Pang [:epang] UX]

(In reply to Ben Francis [:benfrancis] from comment #10)
> (In reply to Eric Pang [:epang] from comment #9)
> > Hi, I've uploaded a visual spec here:
> > https://drive.google.com/open?id=0B9o41Fwl8FagSXRNYUozb0NmWGM
> 
> Thanks Eric, this looks good. Heads up that changes to the interaction
> design may be incoming, but hopefully a lot of your work is re-usable
> https://docs.google.com/presentation/d/1Xja64Radti_xz-WMoI1hmixkXPSTb4xjdxnr-
> PvzHhM/pub?start=false&loop=false&delayms=3000
> 
> The one thing I'm not sure about with your visual spec is the "scrim" (as
> Francis calls them) which obscures content and makes the doorhanger a lot
> like a modal dialog. We're considering using the door hanger for
> informational prompts which the user may want to ignore so I'm a bit
> concerned that obscuring the whole screen might be a bit jarring?
> 
> As a data point, the door hangers in Firefox and Firefox for Android don't
> use a translucent scrim. They do allow you to click outside the door hanger
> to dismiss it, but the scrim is completely transparent.
> 
> Also, the subtle low-contrast distinction between button and background
> colour reminds me that I really need to replace my monitors! :P

Thanks for the feedback Ben! :)

Yes, I'll be able to take the styling and apply it to the updated interaction design.

I'm good to remove the scrim, but it would be great to keep the tapping outside of the door hanger or the icon to dismiss it.  I'll also see if we can use a brighter colour for the pin button!

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Eric Pang [:epang] from comment #9)
> Image assets can be found here:
> https://drive.google.com/
> open?id=0B9o41Fwl8FagflI5bElUUERjYXNERUpTckhWV2Y1QjVBdDE5cTJyTUFrM0ljWkdrcXVK
> UG8

Eric, it looks like the background on these assets is white instead of transparent. Also, can you put these someplace public rather than in a place that requires a Mozilla email address? Attaching a zip file to this bug should do the job. Public dropbox link or something would also work.

Comment 13

[Eric Pang [:epang] UX]

Attachment: Icons-assets.zip

Hey Michael,

Sorry about that!  I think it was just the arrows - I've updated them now.  Let me know if you notice anything else needs to be updated. Thanks!

Comment 14

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [screenshot] doorhanger clipped

One thing we need to consider for visual is what happens when there is a browser history. We simply cannot keep those dimensions for the doorhanger and still line it up under the icon. What should we do in this case?

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

Also, Eric, the padding around the ssl/ssl_broken icons has changed in the image which makes it hard to get your images in since they break in other places like the search bar. Can we keep the same padding here:

https://raw.githubusercontent.com/mozilla-b2g/gaia/master/apps/system/style/chrome/images/light/ssl.png

Comment 16

[GitHub Autolander Bot]

Attachment: [gaia] mikehenrty:bug-1168955-preview-site > mozilla-b2g:master

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8648321 [details] [review]
[gaia] mikehenrty:bug-1168955-preview-site > mozilla-b2g:master

Ben, could you give me some feedback here? I left out the scrim, which means the contents underneath are still actionable. But we only dismiss the door hanger once the user actually scrolls. Is this what the interaction spec is calling for?

Comment 18

[Eric Pang [:epang] UX]

Attachment: ssl_lock_assets.zip

Hey Michael, I've updated the ssl lock icons to have padding and attached.  We talked about this already but the dialog should always stay in place while the arrow is fluid and moves with the site icon :).  Thanks!

Comment 19

[Eric Pang [:epang] UX]

Attachment: ssl_lock_assets.zip

added the light version of the ssl locks

Comment 20

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8648321 [details] [review]
[gaia] mikehenrty:bug-1168955-preview-site > mozilla-b2g:master

Removing fb?, talked with benfrancis on IRC and I have clarity now.

Comment 21

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8648321 [details] [review]
[gaia] mikehenrty:bug-1168955-preview-site > mozilla-b2g:master

Review time fellas.

Comment 22

[Alberto Pastor [:albertopq]]

Comment on attachment 8648321 [details] [review]
[gaia] mikehenrty:bug-1168955-preview-site > mozilla-b2g:master

The code looks good to me. The only thing I would add, is closing the door hanger when focusing on the rocketbar. Otherwise, you can navigate to a new page and see the progressbar in top of the arrow. Happy to fix it in a follow up, though. Thanks!

Comment 23

[Eric Pang [:epang] UX]

Comment on attachment 8648321 [details] [review]
[gaia] mikehenrty:bug-1168955-preview-site > mozilla-b2g:master

Thanks Michael, looking good but just need a few small changes

R- since a few minor updates are needed.

1. Reduce the title font weight to CSS 500

2. Use the title of the page instead of the url under the site icon

3. From and URL should all have the same colour (#858585), weight (CSS 400), Size 14px and all italic. 

4. From Chris' comment in the meeting is it possible to add in a transition when opening and closing the door hanger?

I was thinking we can use the ones defined for dialogs in the web components (maybe with a drop down instead of up).  You'll see what I mean if you look at the ones here :)
http://gaia-components.github.io/gaia-components/

Flag me again when ready and I'll take a look, thanks again Michael!

Comment 24

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8648321 [details] [review]
[gaia] mikehenrty:bug-1168955-preview-site > mozilla-b2g:master

(In reply to Eric Pang [:epang] from comment #23)
> 1. Reduce the title font weight to CSS 500

Done, it was at 400, so I put it to 500.

> 2. Use the title of the page instead of the url under the site icon

This is actually not to spec. We don't want individual page titles since we are pinning an entire site rather than the page. We want the site title. When the site has a site name defined in the manifest (e.g. the gaurdian), we will use that. Otherwise we fall back to the site's origin.

> 3. From and URL should all have the same colour (#858585), weight (CSS 400),
> Size 14px and all italic. 

Done.

> 
> 4. From Chris' comment in the meeting is it possible to add in a transition
> when opening and closing the door hanger?

I originally had a transition similar to the hamburger menu in Firefox Desktop, but I thought the quick fade in was cleaner. In any case, I added that transition back in. Let me know what you think.

Comment 25

[Eric Pang [:epang] UX]

Comment on attachment 8648321 [details] [review]
[gaia] mikehenrty:bug-1168955-preview-site > mozilla-b2g:master

R+ Awesome, thanks for all the changes Mike!

Comment 26

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/fa15462b29258fdec8329bfc367e590022dbc9e5

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Mulet Gij seems unhappy with this:
https://treeherder.mozilla.org/logviewer.html#?job_id=2634427&repo=b2g-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=2634540&repo=b2g-inbound

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Reverted.
https://github.com/mozilla-b2g/gaia/commit/7a77326ef8b14ddef4d101efc336d7b26670ef94

Comment 29

[GitHub Autolander Bot]

Attachment: [gaia] mikehenrty:bug-1168955-preview-site-reland > mozilla-b2g:master

Comment 30

[Michael Henretty [:mikehenrty][:mhenretty]]

Not sure why that failed on b-i but not on the PR. Investigating now.

Comment 31

[Michael Henretty [:mikehenrty][:mhenretty]]

This is still passing on the PR:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=41fa8a4d82e3bcab7e7c0e16e45f429552a346a3

Must be some difference between gaia and b-i

Comment 32

[Michael Henretty [:mikehenrty][:mhenretty]]

I can reproduce locally now, but it's intermittent. 

* thread #1: tid = 0x3e4fa, 0x00000001043db9c9 XUL`mozilla::DisplayItemClip::IntersectWith(mozilla::DisplayItemClip const&) + 409, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x00000001043db9c9 XUL`mozilla::DisplayItemClip::IntersectWith(mozilla::DisplayItemClip const&) + 409
    frame #1: 0x00000001043e6e37 XUL`mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) + 903
    frame #2: 0x00000001043edd35 XUL`mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4 const*, unsigned int) + 8533
    frame #3: 0x000000010444b177 XUL`nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) + 999
    frame #4: 0x000000010447ca2c XUL`nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) + 4780
    frame #5: 0x000000010449c346 XUL`PresShell::RenderDocument(nsRect const&, unsigned int, unsigned int, gfxContext*) + 1414
    frame #6: 0x0000000103a4393e XUL`mozilla::dom::CanvasRenderingContext2D::DrawWindow(nsGlobalWindow&, double, double, double, double, nsAString_internal const&, unsigned int, mozilla::ErrorResult&) + 1374
    frame #7: 0x0000000103644fad XUL`mozilla::dom::CanvasRenderingContext2DBinding::drawWindow(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) + 1293
    frame #8: 0x0000000103a06eb1 XUL`mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) + 401
    frame #9: 0x000000010547ef9c XUL`js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 444
    frame #10: 0x000000010549082d XUL`Interpret(JSContext*, js::RunState&) + 35629
    frame #11: 0x0000000105487cbd XUL`js::RunScript(JSContext*, js::RunState&) + 381
    frame #12: 0x000000010547f3c5 XUL`js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 1509
    frame #13: 0x0000000105468042 XUL`js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 610
    frame #14: 0x00000001057f1fef XUL`JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) + 111
    frame #15: 0x0000000102c365b2 XUL`nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) + 5250
    frame #16: 0x000000010246c230 XUL`PrepareAndDispatch + 576
    frame #17: 0x000000010246b07b XUL`SharedStub + 91
    frame #18: 0x000000010241340a XUL`(anonymous namespace)::MessageLoopIdleTask::Run() + 58
    frame #19: 0x00000001024134a7 XUL`(anonymous namespace)::MessageLoopTimerCallback::Notify(nsITimer*) + 23
    frame #20: 0x0000000102467322 XUL`nsTimerImpl::Fire() + 994
    frame #21: 0x000000010245af47 XUL`nsTimerEvent::Run() + 215
    frame #22: 0x000000010245ee11 XUL`nsThread::ProcessNextEvent(bool, bool*) + 1041
    frame #23: 0x0000000102488301 XUL`NS_ProcessPendingEvents(nsIThread*, unsigned int) + 81
    frame #24: 0x000000010419d874 XUL`nsBaseAppShell::NativeEventCallback() + 116
    frame #25: 0x00000001041f7509 XUL`nsAppShell::ProcessGeckoEvents(void*) + 297
    frame #26: 0x00007fff8cc6ca01 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #27: 0x00007fff8cc5eb8d CoreFoundation`__CFRunLoopDoSources0 + 269
    frame #28: 0x00007fff8cc5e1bf CoreFoundation`__CFRunLoopRun + 927
    frame #29: 0x00007fff8cc5dbd8 CoreFoundation`CFRunLoopRunSpecific + 296
    frame #30: 0x00007fff8f3a356f HIToolbox`RunCurrentEventLoopInMode + 235
    frame #31: 0x00007fff8f3a32ea HIToolbox`ReceiveNextEventCommon + 431
    frame #32: 0x00007fff8f3a312b HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
    frame #33: 0x00007fff947de8ab AppKit`_DPSNextEvent + 978
    frame #34: 0x00007fff947dde58 AppKit`-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 346
    frame #35: 0x00000001041f6ae6 XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 86
    frame #36: 0x00007fff947d3af3 AppKit`-[NSApplication run] + 594
    frame #37: 0x00000001041f7c8c XUL`nsAppShell::Run() + 236
    frame #38: 0x0000000104ade349 XUL`nsAppStartup::Run() + 41
    frame #39: 0x0000000104b3748d XUL`XREMain::XRE_mainRun() + 3325
    frame #40: 0x0000000104b37835 XUL`XREMain::XRE_main(int, char**, nsXREAppData const*) + 645
    frame #41: 0x0000000104b37c43 XUL`XRE_main + 227
    frame #42: 0x0000000100001bfd firefox`main + 1773
    frame #43: 0x00000001000011b4 firefox`start + 52

Comment 33

[Michael Henretty [:mikehenrty][:mhenretty]]

Filed bug 1200580 about the crash.

Comment 34

[Michael Henretty [:mikehenrty][:mhenretty]]

Looks like the crash is bug 1181135. If we disable apz, I can't reproduce the crash anymore locally. So I'm disabling apz for this test and moving forward with relanding.

Comment 35

[Michael Henretty [:mikehenrty][:mhenretty]]

merged back in master with apz disabled on the test: https://github.com/mozilla-b2g/gaia/commit/5aa7b14d5d652f0821a7be9591caa50051cdd97b

Watching b-i to see if it sticks.

Comment 36

[Yura Zenevich [:yzen] (please use "needinfo")]

Just want to bring to your attention that making things hidden with opacity 0 and pointer-events none always breaks accessibility (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/chrome/chrome.css#L335-L338). Please use visibility hidden for cases like this.

Comment 37

[Alberto Pastor [:albertopq]]

Already reviewed bug 1212007. Thanks for fixing this.

Comment 38

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Yura Zenevich [:yzen] from comment #36)
> Just want to bring to your attention that making things hidden with opacity
> 0 and pointer-events none always breaks accessibility
> (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/chrome/
> chrome.css#L335-L338). Please use visibility hidden for cases like this.

Thanks for the fix! I'll keep that in mind.