1009351 - Add link text to mozbrowser context menu data

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Dale Harvey (:daleharvey) PTO until Jan 2020]

When saving a link to the homescreen, we need some type of description for the link, we should add the text contained within the link to the reported data

Comment 1

[Dale Harvey (:daleharvey) PTO until Jan 2020]

So theres another issue with this, we can use the link text as the homescreen title, but it will still have a blank icon, the only way round this I can see is to load the url in the background (possibly in the bookmark app) and fetch the icon, but that seems to be asking a lot (tarako cant really open 2 concurrent browing windows)

The homescreen may be able to xhr index.html and parse it without having to load the external resources, that will significantly reduce the load, but getting into unknown territory here.

Comment 2

[Dale Harvey (:daleharvey) PTO until Jan 2020]

Needinfoing Ehsan as you volunteered yourself on the mailing list :)

Comment 3

[:ehsan akhgari]

(In reply to Dale Harvey (:daleharvey) from comment #2)
> Needinfoing Ehsan as you volunteered yourself on the mailing list :)

Andrew is looking for an owner.

Comment 4

[Dale Harvey (:daleharvey) PTO until Jan 2020]

Laying out the options

1. Push back on the feature - People arent as likely to bookmark pages they havent visited, you can save the current page to the homescreen

2. Use link text, blank icon - Easy, really confusing as all app icons will be generic, will be bookmarking a link that possibly redirects, link text may be meaningless

3. As 2, but update blank bookmarks that you visit - Will be fairly complex to update bookmarks, may run into implementation issues, confusing for user as their bookmarks change, I dont think it will work on links that redirect

4. Load link in mozbrowser - Hugely expensive on memory / cpu / network, I think expensive enough to not consider, straight up wont work on tarako

5. Load link url and parse html / dont load external resource - As email, will be complex to implement

Comment 5

[Dale Harvey (:daleharvey) PTO until Jan 2020]

So I checked what happens with firefox when we do this

1. The bookmark is always saved with the link text, the text isnt updated when the page is visited
2. If it is an unvisited page, it is stored with a blank icon and the icon will be updated when we visit the page, If we have previously visited the page the original icon will be picked up

I will look into the viability of having bookmarks keep their icons in sync with their equivalent places data

Comment 6

[Dale Harvey (:daleharvey) PTO until Jan 2020]

Christian, what do you think about this, off the top of my head

When the homescreen sees a null icon, it tries to read the icon from the places store, it also needs a listener that ensures if a currently bookmarked url is changed in places, it should be updated.

I dont think that should be that complicated, and it looks like picking the link text is enough, so can split those out into seperate bugs.

Comment 7

[Cristian Rodriguez (:crdlc)]

I think that we could do it:

When the bookmark is updated in places store we could automatically update this info in bookmarks store and then, homescreen apps will receive an event saying "xx bookmark has been updated" and they could paint the correct icon for free without adding any code in homescreen apps. So we'll avoid that homes have to access to places store thought

Is it feasible or did I miss something?

Comment 8

[Dale Harvey (:daleharvey) PTO until Jan 2020]

That is what I first thought, however will the bookmark app be running? I thought it was mostly only alive whenever people go through the bookmark activities, I would prefer to keep this code out of the homescreen if possible

Comment 9

[Andrew Overholt [:overholt]]

I've confirmed with Kan-Ru that he is the owner of mozbrowser.

Comment 10

[Dale Harvey (:daleharvey) PTO until Jan 2020]

Christian

As above, the bookmark app would need to be running in order to see the update to the places store and propogate that change to the homescreen wont and right now it will only run on user triggering activities right?

Comment 11

[Cristian Rodriguez (:crdlc)]

Yes, bookmark app is only running when users trigger an activity

Comment 12

[Dale Harvey (:daleharvey) PTO until Jan 2020]

Talked about this with Christian, we can import the shared bookmarks library into system app and have that update the bookmarks manually on page load, the system app can also pick up previously visited icons before triggering the save homescreen activity.

I think we will need to start taking a look at merging the places / bookmarks as Ben has mention, this is duplication of data but more important its extra work being done on every page load which is a performance critical time, but future plans, this will work for now

Comment 13

[Kan-Ru Chen [:kanru] (UTC+8)]

The link text could be a quite complex sub DOM. We would have to return only the text representation. Do you think we should try to limit the maximum characters returned?

Comment 14

[Dale Harvey (:daleharvey) PTO until Jan 2020]

textContent will already do that for us I believe, as for limiting, would probably be a sensible idea

Comment 15

[Kan-Ru Chen [:kanru] (UTC+8)]

Attachment: Add link text to mozbrowser context menu data

Comment 16

[Olli Pettay [:smaug]]

Comment on attachment 8439850 [details] [diff] [review]
Add link text to mozbrowser context menu data

This is fine, or perhaps we should limit the length of .text, so that we don't
accidentally send huge amount data from child to parent.
elem.textContent.substring(0, 128); or some such?

Comment 17

[Kan-Ru Chen [:kanru] (UTC+8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7490b2dbc69c

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7490b2dbc69c

Comment 19

[Chris Mills (Mozilla, MDN editor) [:cmills]]

I believe the documentation is now complete, see here:

https://developer.mozilla.org/en-US/docs/Web/Events/mozbrowsercontextmenu#The_MenuSystem_object

The text property is what you are on about, right? Let me know if I'm wrong.