Cork Reporter
	Description • 10 years ago

The dropdown arrow in the urlbar is missing in gtk3 builds

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 1 • 10 years ago

Yes, I can see that. But it does not look like a rendering bug - the whole element is missing and I can't click there.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 2 • 10 years ago

The element is here in latest trunk (174984:fa098f9fe89c) but the color of pressed one is wrong (white).

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 3 • 10 years ago

Actually the color of active arrow is correct, because the theme expect dark (blue) background - like arrows in menu. The workaround would be to render the arrow always in a normal state or use an arrow from combo box, which is similar to this one.

	Amit Prakash Ambasta
	Comment 4 • 10 years ago

I still don't see this on the latest pull from https://github.com/mozilla/gecko-dev.git

Verified that the latest commit to hg.mozilla.org is the same.

	Cork Reporter
	Comment 5 • 10 years ago

Did a new test[1] and i still don't see any dropdown arrow at all. The entire element seams to be missing.

[1] http://hg.mozilla.org/mozilla-central/df7b26e90378

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 6 • 10 years ago

Yes, I can reproduce it on my other system.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 7 • 10 years ago

Well, it's this one:

  <xul:dropmarker anonid="historydropmarker"
                  class="autocomplete-history-dropmarker urlbar-history-dropmarker"
                  allowevents="true"
                  xbl:inherits="open,enablehistory,parentfocused=focused"/>

from urlbarBindings.xml but it's a mystery why it's not rendered...

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 8 • 10 years ago

.urlbar-history-dropmarker {
  -moz-appearance: toolbarbutton-dropdown;
}

Looks like the urlbar-history-dropmarker/toolbarbutton-dropdown class is not rendered. May be related to different gtk3 icons set or so.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 9 • 10 years ago

The .autocomplete-history-dropmarker class seems to be missing in gtk3 build.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 10 • 10 years ago

The issue is that size of the arrow (got by gtk_widget_size_request) is 0x0 so it's not rendered.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 11 • 10 years ago

It's similar to the combobox issue - the inner arrow can be inserted as a child to container but needs to be created with the widget.

	Karl Tomlinson (:karlt)
	Comment 12 • 10 years ago

Comment on attachment 8402726 [details] [diff] [review]
patch

># HG changeset patch
># Parent 4941a2ac0786109b08856738019b016a6c5a66a6
># User Martin Stransky <stransky@redhat.com>
>Bug 984075 - [Gtk3] Missing dropdown arrow in urlbar, r=?karlt

The commit message needs to say what is changed, not describe the bug being fixed.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

It is usually helpful to also say why it is being changed and why it fixes a problem.  i.e. explain about the incorrect size.  This can go in a second paragraph if it doesn't fit in the first line.

> static gint
> ensure_toggle_button_widget()
> {
>     if (!gToggleButtonWidget) {
>-        gToggleButtonWidget = gtk_toggle_button_new();
>+        gToggleButtonWidget = gtk_menu_button_new();

A GtkMenuButton sounds appropriate for the urlbar, but not for a variable named "gToggleButtonWidget".  Please rename the variable and function, assuming it is not used for any other purpose.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 13 • 10 years ago

Thanks! This one should address it.

	Karl Tomlinson (:karlt)
	Comment 14 • 10 years ago

Comment on attachment 8403287 [details] [diff] [review]
patch v.2

>Bug 984075 - Extract arrow widget from GtkMenuButton widget instead of creating
>it directly which gives us propper arrow size. r=?karlt

Yes, this is good message.

s/propper/proper/

Please put this all on one long line, or rearrange to leave a blank line after the reviewer and before a new sentence.

>-ensure_toggle_button_widget()
>+ensure_toggle_menu_button_widget()
> {
>-    if (!gToggleButtonWidget) {
>+    if (!gToggleMenuButtonWidget) {

I would have called these ensure_menu_button_widget and gMenuButtonWidget, but including the "toggle" is OK if you think that is helpful.

Sorry, I didn't realize that a MenuButtonWidget is a ToggleButtonWidget, but I think it is less confusing to remember that this is a MenuButtonWidget.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 15 • 10 years ago

Updated, thanks. 

> I would have called these ensure_menu_button_widget and gMenuButtonWidget,
> but including the "toggle" is OK if you think that is helpful.

I like the "toogle", it self-explanatory and we have other long names in the file.

	Carsten Book [:Tomcat]
	Comment 16 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bdbe71f21b2

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/3bdbe71f21b2

	Cork Reporter
	Comment 18 • 10 years ago

Verified with http://hg.mozilla.org/mozilla-central/rev/b79c2995d25e