1235121 - pin card does not show correctly when page defined theme-color

Status: RESOLVED FIXED

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

Description

[Alison Shiue]

STR:
1. Pin a page which defined some meta info and theme-color, ex: http://alison-shiue.github.io/pin_article/with_meta_theme_color.html
2. Go to homescreen to check the pin card

Expected result:
The pin card shows correctly

Actual result:
The pin card does not show as expected

Build info:
[Flame]
Build ID               20151223121205
Gaia Revision          94cc99aac11339e297da186ec7153ca4ae9ab8cd
Gaia Date              2015-12-23 06:07:52
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/35b211eaad1fa828064514c547057e4400e24459
Gecko Version          46.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151223.112946
Firmware Date          Wed Dec 23 11:29:56 UTC 2015
Bootloader             L1TC000118D0

[Aries]
Build ID               20151223121756
Gaia Revision          94cc99aac11339e297da186ec7153ca4ae9ab8cd
Gaia Date              2015-12-23 06:07:52
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/35b211eaad1fa828064514c547057e4400e24459
Gecko Version          46.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151223.113503
Firmware Date          Wed Dec 23 11:35:11 UTC 2015
Bootloader             s1

Comment 1

[Alison Shiue]

Hi Alberto,

It's weird that the pin cards on different devices have different results. 
For Aries, unable to show the title; for Flame, unable to show title, icon, and the image does not apply the theme color.

Could you help to check this issue? Thank you.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1235121-pin-card > mozilla-b2g:master

Comment 3

[Alberto Pastor [:albertopq]]

Alison, could you please verify that the submitted patch fixes the issue in both devices?

Thanks!

Comment 4

[Alberto Pastor [:albertopq]]

Comment on attachment 8707481 [details] [review]
[gaia] albertopq:1235121-pin-card > mozilla-b2g:master

Tricky one! I finally managed to create a test that currently fails but it doesn't after the patch.

The current issue is that if we call editPlace while writeInProgress === true, it recursively calls editPlace again, but the changes were removed in the previous call ([1]). We want to remove the pending changes strictly only when the data has been successfully stored in the DataStore. 

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/places.js#L579

Comment 5

[Alison Shiue]

Attachment: pin_page_without_title.png

Hello Alberto, 

I cannot reproduce this issue both on Flame and Aries with this patch!
However, when pinned a page without title, URL would be shown on the pinned card. 
Could you please help to check this? Thank you.

Comment 6

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

Waiting for comment 5 to be resolved (either in patch or with a new bug) before doing review.

Comment 7

[Alberto Pastor [:albertopq]]

I'm not sure if that's the expected behavior. In the same way we show the URL instead of the title in the app chrome, when we don't have one. Ben, what do you think? Should we show the URL as the title in the PIN card when we don't have specific title or og:title?

Comment 8

[Ben Francis [:benfrancis]]

Good question, and yes I think we probably show the URL if no page title is provided.

Comment 9

[Alison Shiue]

ok, I think we need to update the visual spec for this decision.
Hi Eric, can you help on it?

Comment 10

[Eric Pang [:epang] UX]

Hi Alison,
I've updated the visual spec here: https://mozilla.box.com/s/0nsx2y0ggihcu2ucu0s6x4xxithtkst5

On Pg: 32 I've added two more scenarios for articles 
#6 - Covers what happens when there's no image and title/header
a. In this case a screenshot of the page should be taken (we should always have an image) with the meta-theme colour overlay - if no meta-theme is available it defaults to #4d4d4d at 15% (as shown in #8)
b. In place of the title the url is used: Font Size: 10px, Medium Italic (CSS 500)

#7 - Covers the same scenario as above but without descriptive text
a. Same as above with the url being vertically centered in the space

flag or ping me if you have any questions! :)

Comment 11

[Alison Shiue]

(In reply to Eric Pang [:epang] UX from comment #10)
> Hi Alison,
> I've updated the visual spec here:
> https://mozilla.box.com/s/0nsx2y0ggihcu2ucu0s6x4xxithtkst5
> 
> On Pg: 32 I've added two more scenarios for articles 
> #6 - Covers what happens when there's no image and title/header
> a. In this case a screenshot of the page should be taken (we should always
> have an image) with the meta-theme colour overlay - if no meta-theme is
> available it defaults to #4d4d4d at 15% (as shown in #8)
> b. In place of the title the url is used: Font Size: 10px, Medium Italic
> (CSS 500)
> 
> #7 - Covers the same scenario as above but without descriptive text
> a. Same as above with the url being vertically centered in the space
> 
> flag or ping me if you have any questions! :)

Hi Eric, is the spec link correct? I cannot find the difference between the old and the new version.

Comment 12

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

Comment on attachment 8707481 [details] [review]
[gaia] albertopq:1235121-pin-card > mozilla-b2g:master

I'll give the r+ on the changes to the timing for deleting the working copy of placeChanges. Alberto, I'll leave it up to you if you want to file a new bug for Eric's changes.

Also, good job on the test.

Comment 13

[Alberto Pastor [:albertopq]]

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

Alison, could you please create a new bug with the missing pieces and assign it to me? Thanks!

Comment 14

[Alison Shiue]

(In reply to Alberto Pastor [:albertopq] from comment #13)
> master:
> https://github.com/mozilla-b2g/gaia/commit/
> a1038d0ef8cacff3ca0346924605314f434eb1e5
> 
> Alison, could you please create a new bug with the missing pieces and assign
> it to me? Thanks!

Sure, but I need to know the difference first...
Hi Eric, can you please check if the spec link you provided is correct? Thank you.