173223 - Separator remains on toolbar when home button disabled

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Andrei Leo]

Mozilla build 2002100704 Win32
Win2k

Steps to reproduce
1.Clean install. No Mozilla directory, no user directory
2.Run Mozilla. Go to preferences and unchek Home button in Navigator preferences.

Result:
There is a separator on the toolbar even there nothing to separate.

Expected:
Separator should never appear as a first item on the toolbar


Screenshot will follow.

Comment 1

[Andrei Leo]

Attachment: Modern theme screenshot

Classic theme is also affected

Comment 2

[José Jeria]

is this a dupe of 50226?

Comment 3

[José Jeria]

Forget my last comment. Wrong bug.

Comment 4

[José Jeria]

Reassigning to default owner.

Comment 5

[Andrei Leo]

this setting in userChrome.css removes this annoyance

#home-button[hidden="true"] + toolbarseparator {display:none !important}

Toolbar refresh after enabling/disabling home button in preferences is a bit slow.
For me it takes about 2 sec to hide/show separator.

Comment 6

[José Jeria]

Andrei, about the 2 second delay, you mean always or after changing the
userChrome.css?

Comment 7

[Andrei Leo]

José,

delay (about 2-3 seconds) is  from the moment when I disable home button in
preferences and the moment when separator finally disappears from toolbar.
("home" button itself disappears instantly)

0. there is a "home" button and separator on the toolbar
1. go to prefrences and disable "home" button
2. "home" button dissapered from toobar. separator still there.
3. wait 2-3 seconds
4. separator is gone

The same when I enable home button. First home button appears on the toolbar and
then separator appears after couple of seconds.

Comment 8

[Andrei Leo]

Sorry. I forgot to mention that delay is only when userChrom.css is tweeked as I
described in comment #5

Comment 9

[Martin Wulffeld]

Taking. Have fix soon.

Comment 10

[Martin Wulffeld]

Attachment: Patch

This broke because the first child didn't have a hidden attribute and the loop
therefore returned false immediately.

There's no reason to call hasAttribute() here since getAttribute() will return
an empty string if an element doesn't have the hidden attribute.

Comment 11

[Andrei Leo]

I suppose this should work better :)

-    if(buttonNode.getAttribute("hidden") == "false")
+    if(buttonNode.getAttribute("hidden") != "true")

because if getAttribute() returns empty string when there is no hidden attribute
and "" != "false" it means that function will return true if first child IS
visible. Which is wrong I assume.

Comment 12

[Andrei Leo]

one more update...

-    if(buttonNode.getAttribute("hidden") != "true")
+    if(buttonNode.tagName == "toolbarbutton" && 
+       buttonNode.getAttribute("hidden") != "true")

because should not worry about "tooltip" and other non-button elements

Comment 13

[Martin Wulffeld]

First of all. I haven't worked much with JS or XUL so I might wrongly assume
that ALL widgets (I don't consider a tooltip a widget) have a hidden attribute
at all time set to either false or true. If this is wrong please enlighten me :)

Regarding comment #11.
This wouldn't work since the tooltip doesn't have a "hidden" attribute and
therefore we would return false which is wrong. Also I think it's better to test
explicitly on what you expect. In this case use == instead of != since we're
looking for 'false'.

Regarding comment #12.
If we should test if it's a button I think we should test if it's any element
that can be on a toolbar. So this includes textfields, radio buttons and so on
since we don't know what might be thrown at this toolbar in the future. In that
case we should simply test if the element has the "hidden" attribute. And so I
believe we're back to my patch since we just need to test if hidden=false. How's
that for a cyclic comment? :)

Sorry, if I'm completely out in the woods here. As I said I'm assuming the
hidden attrib is always present on visible elements whether they're shown or not.

Maybe the function could be renamed to allSideWidgetsAreHidden() and take an
element argument from which it searched from so it could check a right side as
well. Just a thought.

Comment 14

[Andrei Leo]

Martin,

I think your are right. :) 
I'm very confused. Something got mixed up in my head.

Comment 15

[jag (Peter Annema)]

In this case, the comparison getAttribute("hidden") == "false" is okay since we
need to have a value assigned to it for that attribute and its value to be
correctly persisted (stored in localstore.rdf and restored the next time the
document is loaded), but I'd recommend the idiom getAttribute("hidden") !=
"true", since that's the right test in the (common) case where the value "false"
and the absence of the attribute mean the same thing and both uses are used
mixedly, and since it'll work just fine in this case it's simpler for readers of
the code to see "the right thing" is being tested for. Also, instead of testing
for the element to be a button or any of the others that could appear on the
toolbar, you could test to ignore all the elements you don't care about, in this
case el.localName != "tooltip".

Comment 16

[Martin Wulffeld]

Attachment: Patch

Test on != true and ignores only tooltips so far.

Comment 17

[jag (Peter Annema)]

Comment on attachment 112135 [details] [diff] [review]
Patch

sr=jag

Comment 18

[[e-mail]]

Comment on attachment 112135 [details] [diff] [review]
Patch

Rather than coding around it, why not just put the tooltip somewhere else in
the document?

>+        if(buttonNode.getAttribute("hidden") != "true")
What's wrong with if (!buttonNode.hidden)?

Also, if you are going to touch the code, I would start at the
home-bm-separator and work backwards (so as to avoid looking at the tag name).
Otherwise I would have been consistent about tagName vs localName... although I
wouldn't have looked at the name twice anyway.

Comment 19

[Martin Wulffeld]

Attachment: Patch v3

>Rather than coding around it, why not just put the tooltip somewhere else in
>the document?

I believe that's the wrong thing to do. If another button and associated
tooltip is added to the wrong place in navigator.xul we need to sort this bug
out once more. Also in .xul the tooltip logically belongs near the button it's
associated with.

>+	  if(buttonNode.getAttribute("hidden") != "true")
>What's wrong with if (!buttonNode.hidden)?

I just followed previous style. Fixed.

>Also, if you are going to touch the code, I would start at the
>home-bm-separator and work backwards (so as to avoid looking at the tag name).

>Otherwise I would have been consistent about tagName vs localName... although
I
>wouldn't have looked at the name twice anyway.

Agree. Fixed.

Comment 20

[Martin Wulffeld]

Attachment: Patch v4

Simplified patch based on Neil's comments on IRC.

Comment 21

[jag (Peter Annema)]

Comment on attachment 112851 [details] [diff] [review]
Patch v4

Erh, shouldn't we be using .localName instead of .tagName?

And hrm... Theoretically the tooltip could move elsewhere. I wanted to put it
inside the button, but for some reason it wouldn't show up if I did that. I
just tried again and it works fine now, so another fix for this bug would be to
simply move the tooltip into the toolbarbutton. Patch coming up.

Comment 22

[jag (Peter Annema)]

Comment on attachment 112851 [details] [diff] [review]
Patch v4

Oh, wait... I remember now. The tooltip would show up, but the links inside it
would be underlined 'coz they'd get the bookmark item style applied to them.
That's why I didn't put them in. I guess we could still do that, and fix up the
skins to explicitely not apply that style to labels inside tooltips inside
toolbarbuttons or something. For now however, let's go with your patch, but fix
tagName to be localName.

Comment 23

[Martin Wulffeld]

Attachment: Patch v5

Uses localName instead of tagName.

Comment 24

[Martin Wulffeld]

Comment on attachment 112953 [details] [diff] [review]
Patch v5

The separator on the personal toolbar didn't go away if the home button was
hidden due to a regression (an added tooltip). This patch fixes the function
which tests if all elements left of the separator are hidden ignoring tooltips.

Has been tested successfully on Linux and Windows builds.

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 112953 [details] [diff] [review]
Patch v5

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 26

[[e-mail]]

Fix checked in.

Comment 27

[José Jeria]

Attachment: Screenshot

See screenshot, the separator still remains with build 2003013008. Though it
goes away when you:
1. add the home button.
2. remove the home button

So people that dont do that will have the separator like the screenshot

Comment 28

[Asa Dotzler [:asa]]

Re-opening. This isn't completely fixed. Neil, can you take another look at this?

Comment 29

[[e-mail]]

Does this apply to people who hid the home button in the past, and still have
the extraneous separator? This code unfortnately only takes effect as the home
button is hidden, so those people will have to use the described workaround.

Comment 30

[Martin Wulffeld]

Neil, yes it does only apply to people hid the home button in the past because
the hidden value is stored in localstore.rdf. This code here is never called at
startup as you say.

I could add a check for the hidden attrib to checkForDefaultBrowser() or some
other function which is called at startup. Not nice IMHO but that's your call?

Comment 31

[[e-mail]]

You'll never get anything that would add to Ts or Txul checked in.
This is as fixed as it will ever get.