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

RESOLVED FIXED in 2.6 S5 - 1/15

Status

Firefox OS
Gaia::System::Browser Chrome
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: Alison Shiue, Assigned: albertopq)

Tracking

({polish})

unspecified
2.6 S5 - 1/15
ARM
Gonk (Firefox OS)
polish

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe], URL)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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.
QA Whiteboard: [COM=Pin the Web]
Flags: needinfo?(apastor)
(Reporter)

Updated

2 years ago
Flags: needinfo?(apastor)
Whiteboard: [systemsfe]
(Reporter)

Updated

2 years ago
Flags: needinfo?(apastor)
Keywords: polish
(Assignee)

Updated

2 years ago
Assignee: nobody → apastor
Flags: needinfo?(apastor)
Target Milestone: --- → 2.6 S5 - 1/15
Created attachment 8707481 [details] [review]
[gaia] albertopq:1235121-pin-card > mozilla-b2g:master
(Assignee)

Comment 3

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

Thanks!
Flags: needinfo?(ashiue)
(Assignee)

Comment 4

2 years ago
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
Attachment #8707481 - Flags: review?(mhenretty)
(Reporter)

Comment 5

2 years ago
Created attachment 8708169 [details]
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.
Flags: needinfo?(ashiue) → needinfo?(apastor)
Waiting for comment 5 to be resolved (either in patch or with a new bug) before doing review.
(Assignee)

Comment 7

2 years ago
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?
Flags: needinfo?(apastor) → needinfo?(bfrancis)
Good question, and yes I think we probably show the URL if no page title is provided.
Flags: needinfo?(bfrancis)
(Reporter)

Comment 9

2 years ago
ok, I think we need to update the visual spec for this decision.
Hi Eric, can you help on it?
Flags: needinfo?(epang)
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! :)
Flags: needinfo?(epang)
(Reporter)

Comment 11

2 years ago
(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.
Flags: needinfo?(epang)
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.
Attachment #8707481 - Flags: review?(mhenretty) → review+
(Assignee)

Comment 13

2 years ago
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!
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(epang) → needinfo?(ashiue)
Resolution: --- → FIXED
(Reporter)

Comment 14

2 years ago
(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.
Flags: needinfo?(ashiue) → needinfo?(epang)

Updated

6 months ago
Flags: needinfo?(epang)
You need to log in before you can comment on or make changes to this bug.