Closed Bug 414845 Opened 16 years ago Closed 16 years ago

Urlbar history dropmarker pushed state

Categories

(Toolkit :: UI Widgets, defect)

Product:
Toolkit
Component:
UI Widgets
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9beta4

People

(Reporter: frnchfrgg, Assigned: dao)

References

Details

Attachments

(1 file, 3 obsolete files)

Make the urlbar history dropmarker stay pushed when history popup is open (v3)
2.64 KB, patch
enndeakin
: review+
mtschrep
: approval1.9+
Details | Diff | Splinter Review 
We should make the history dropmarker of the urlbar stay pushed as long as the history popup is open (not the autocomplete stuff).

Attached is a patch which achieves that by setting the "open" attribute to "true" on the urlbar textbox whenever the history popup is opened, and removing that attribute when popup is closed (even when closing autocomplete, I suppose, since it's in popuphiding event, but it doesn't matter).
Perhaps this patch needs a comment explaining that the open attribute only corresponds to the history popup state; or perhaps we should set the open attribute only on the dropmarker (and remove the xbl:inherit); please tell me what should be done.
Attachment #300368 - Flags: review?(ventnor.bugzilla)
Status: NEW → ASSIGNED
Strange, choosing "accept bug" doesn't assign the bug to me, just mark it ASSIGNED.
Try to choose accept bug now.
Assignee: nobody → frnchfrgg-mozbugs
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attachment #300368 - Flags: review?(twanno)
Attachment #300368 - Flags: review?(ventnor.bugzilla) → review+
Component: OS Integration → XUL Widgets
Product: Firefox → Toolkit
QA Contact: os.integration → xul.widgets
Comment on attachment 300368 [details] [diff] [review]
Make the urlbar history dropmarker stay pushed when history popup is open

Seth or Neil, can one of you look at this?
Attachment #300368 - Flags: review?(seth)
Attachment #300368 - Flags: review?(enndeakin)
Comment on attachment 300368 [details] [diff] [review]
Make the urlbar history dropmarker stay pushed when history popup is open

throwing over the wall at gavin sharp.

he's probably overloaded, but he'll still get to this before me.

consider also mano.
Attachment #300368 - Flags: review?(seth) → review?(gavin.sharp)
Comment on attachment 300368 [details] [diff] [review]
Make the urlbar history dropmarker stay pushed when history popup is open

Canceling the review request to twanno, since ventron did it faster (it was mainly to lighten ventnor's burden)
Attachment #300368 - Flags: review?(twanno)
Comment on attachment 300368 [details] [diff] [review]
Make the urlbar history dropmarker stay pushed when history popup is open

You only want the button to look depressed when opened via the mouse, but not when opened via a script, or with the keyboard? If this is what you want, then ok, otherwise, you should set the attribute in showHistoryPopup
Attachment #300368 - Flags: review?(enndeakin) → review+
Native controls do show the pushed state when opened via keyboard, so do that here also. The dropmarker DOESN'T appear pushed when the autocomplete is open.
Attachment #300368 - Attachment is obsolete: true
Attachment #300368 - Flags: review?(gavin.sharp)
Comment on attachment 300655 [details] [diff] [review]
Make the urlbar history dropmarker stay pushed when history popup is open

Enn, can you confirm this is what you wanted?
Attachment #300655 - Flags: review?(enndeakin)
Attachment #300655 - Flags: review?(enndeakin) → review+
Comment on attachment 300655 [details] [diff] [review]
Make the urlbar history dropmarker stay pushed when history popup is open

This makes the address bar dropdown button act like most other buttons with regards to pushed state. Simple fix that makes a big, noticeable difference.
Attachment #300655 - Flags: approval1.9b3?
Comment on attachment 300655 [details] [diff] [review]
Make the urlbar history dropmarker stay pushed when history popup is open

a=beltzner
Attachment #300655 - Flags: approval1.9b3? → approval1.9b3+
Keywords: checkin-needed
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.111; previous revision: 1.110
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta3
Was this bug really just for Linux? I'm asking because I don't see this on Windows but I know a lot of times people forget to change flags after the initial filling of the bug to include all OSes.
Blocks: 413819
Comment on attachment 300655 [details] [diff] [review]
Make the urlbar history dropmarker stay pushed when history popup is open

>       <method name="showHistoryPopup">
>         <body><![CDATA[
>+          this.setAttribute("open", "true");

So, since textbox.open = true will call showHistoryPopup(), we're setting the "open" attribute twice.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 415530
Dao, are you sure ? I tried to set the attribute to open manually and the popup wasn't open; I think showHistoryPopup() is called when textbox.open is set to true, not when the attribute "open" of textbox is set to true.
Yes, I was talking about the property setter, which sets the attribute and calls showHistoryPopup, which sets the attribute again.
So what you're saying is that the setAttribute is no longer needed in the property setter ? If so, I'll remove it while correcting bug 415530
Yes
When did this change? Before this patch landed, my dropmarker wouldn't stay open after the initial click. If the open part is removed, won't it regress that?
(In reply to comment #18)
> When did this change?

It didn't. |textbox.open = true| is one way to open the popup, but apparently we didn't use that, at least in the Location Bar, otherwise you would have seen the "pushed state".
(In reply to comment #19)
> |textbox.open = true| is one way to open the popup, but apparently
> we didn't use that, at least in the Location Bar, otherwise you would have seen
> the "pushed state".

So, what patch is going to add that so that "pushed state" works if the other way is going to be removed?
No way is going to removed, but we should remove redundant this.setAttribute("open", "true") calls. Doing that once in the right place is enough to get the "pushed state". FrnchFrgg wrote he would take care of this in bug 415530, so let's close this bug again.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Ah, ok, I understand. Thanks!
Target Milestone: mozilla1.9beta3 → ---
Target Milestone: --- → mozilla1.9beta3
Blocks: 415350
No longer blocks: 415350
Depends on: 415350
Given the regressions this caused I think it might be a good idea to back this out and fix it once we can address the issue mentioned here, and bug 415350/bug 415530. Any objections?
Not from me anyway. There ought to be a better way of solving this. I cannot do the backout, so feel free to do so.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So, back out the entire patch or just a portion of it?
Keywords: checkin-needed
The entire patch.
Backed out.
Keywords: checkin-needed
Target Milestone: mozilla1.9beta3 → ---
Attachment #300655 - Attachment is obsolete: true
I hope you don't mind if I take this bug. Time for non-blocking bugs is running out. Thanks for getting this started anyway.
Assignee: frnchfrgg-mozbugs → dao
Status: REOPENED → ASSIGNED
Attachment #302123 - Flags: review?
Attachment #302123 - Flags: review? → review?(enndeakin)
Attachment #302123 - Flags: review?(enndeakin) → review+
Sorry, I forgot about bug 415350. This is really nasty, and there's one fallout:

 1. disable history and remove all bookmarks
 2. push the dropmarker (nothing happens)
 3. within one second, enable history and open a new page or bookmark a page
 4. within the same second, type into the location bar

Assuming you typed the right thing, you'll now get a popup and the pushed state without having pushed the dropmarker. Probably not too catastrophic, and the steps to reproduce seem impossible to perform :)

Would be better if, for instance, there was a way to get notified when the search ended without a result, but the interfaces don't seem to provide this or something else that could be helpful.
Attachment #302123 - Attachment is obsolete: true
Attachment #302145 - Flags: review?(enndeakin)
Also, if the search does have results and for some reasons it takes more than one second to open the popup, we wouldn't get the pushed state. Not catastrophic either, and shouldn't really happen.
OS: Linux → All
Hardware: PC → All
Dao: No problem; I was going to reassign to nobody, since I didn't really know a good solution. Yours is correct, but the 1 second timer isn't really satisfying. I guess it will do, though, since we're running out of time.
Comment on attachment 302145 [details] [diff] [review]
Make the urlbar history dropmarker stay pushed when history popup is open (v3)

Looks very hacky though.
Attachment #302145 - Flags: review?(enndeakin) → review+
Well, it is a hack.
Attachment #302145 - Flags: approval1.9?
Attachment #302145 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.115; previous revision: 1.114
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9b4pre) Gecko/2008021304 Minefield/3.0b4pre ID:2008021304
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: