967256 - The bookmark button should let you bookmark the page in the viewport irrespective of what's in the URL bar

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect)

Description

[(no longer active)]

STR:

1. Open a new tab.
2. Load a URL that you have previously bookmarked.  The bookmark icon glows.
3. Focus the location bar and cut the URL.  The bookmark icon loses its glow (first bug)
4. Try clicking on the bookmark button, nothing happens (second bug)
5. Press Cmd+Z to undo the cut, that won't help either.

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

Just tested this on win7 x64 latest tinderbox m-c win32 build.

Following the STR I see the same thing. 

However, I see another strange twist.  After step 5 if I switch tabs and then come back to the tab that was used for testing, I see a very faint 'yellow-star'.  You cannot edit or change that bookmark however.

Opening the Library and just deleting that bookmark, closing the tab, the reopening that tab with ctrl+Shit+T the Star is now 'dimmed' and its impossible to re-bookmark that particular site that was used for testing. 

Have not tried re-starting browser yet.

So, is this potential data-loss ?

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

restarting the browser the site I was testing with still has a Dim-star, and cannot be re-bookmarked, or edited.

Comment 3

[Jim Jeffery not reading bug-mail 1/2/11]

Had to delete the entry from History in order to restore the bookmarking function.

Comment 4

[:Gijs (he/him)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #0)
> STR:
> 
> 1. Open a new tab.
> 2. Load a URL that you have previously bookmarked.  The bookmark icon glows.
> 3. Focus the location bar and cut the URL.  The bookmark icon loses its glow
> (first bug)

Actually, it's always worked like that. I'm on beta, and if I edit the URL of a bookmarked page, the star disappears entirely, so steps 4 and 5 don't even work.

> 4. Try clicking on the bookmark button, nothing happens (second bug)
> 5. Press Cmd+Z to undo the cut, that won't help either.

Comment 5

[:Gijs (he/him)]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #1)
> Just tested this on win7 x64 latest tinderbox m-c win32 build.
> 
> Following the STR I see the same thing. 
> 
> However, I see another strange twist.  After step 5 if I switch tabs and
> then come back to the tab that was used for testing, I see a very faint
> 'yellow-star'.  You cannot edit or change that bookmark however.
> 
> Opening the Library and just deleting that bookmark, closing the tab, the
> reopening that tab with ctrl+Shit+T the Star is now 'dimmed' and its
> impossible to re-bookmark that particular site that was used for testing. 


If you just reload the page, the button works fine again. Session store (which handles ctrl-shift-t) also keeps the currently typed URL, and because you could have something completely different in the URL bar (www.foo.org as compared to www.bar.org) it's also really not clear what the desired behaviour here is.

Hiding the star entirely made a lot of sense because it was in the url bar. Now it isn't, so hiding it isn't great, either. Not sure what we want to do here. Madhava?

In the meantime, I don't think this is a regression after all, and I don't think it merits fixing for Australis v1, so I'm putting this down as a P4. Perhaps UX disagrees.

Comment 6

[Marco Bonardo [:mak]]

we are disabling the star when the proxystate is invalid, exactly cause the url shown may be totally different from the one bookmarked... though now that the button is not in the urlbar, we discussed making it related not to the url but to the tab contents, and if that's fine, we may stop checking proxystate and stop disabling the button in such cases.

Comment 7

[YF (Yang)]

The bookmark button will restore vitality if focus to urlbar and press Esc key.

Comment 8

[:Gijs (he/him)]

302 Philipp.

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #8)
> 302 Philipp.

I'm not sure I understand what you mean by 302, but I can answer the question from earlier ;)

Since the star and the URL bar aren't connected anymore, the star should refer to whatever is loaded in the viewport.

Comment 10

[:Gijs (he/him)]

I think this will need broader places changes, so I'm moving it there.

Comment 11

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #10)
> I think this will need broader places changes, so I'm moving it there.

shouldn't be too complex, it's mostly the fact we were in the locationbar, and thus we were listening to the page proxy state.
I think there's also some code disallowing bookmarks for about:blank or such.
We should likely remove all those limitations and simplify the code.

Comment 12

[Marco Bonardo [:mak]]

Considered the Star is again bound to the Address Bar, this bug is back to the start. I'm going to resolve it because now we just hide the star when editing, rather than disabling it