900046 - Changing between tabs causes entire GUI to reload with default motif before applying user motif

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Thorbear]

Attachment: metro.css

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Using a motif that changes other GUI-elements than just the chat, changing between tabs (Ctrl+Tab).
FireFox 22.0
ChatZilla 0.9.90.1
Windows 7 x64


Actual results:

The GUI flashes into the default motif before returning to the user motif, which also makes the whole window "shake".


Expected results:

The transition between tabs should just change the contents of the userlist and chat.

Comment 1

[:Gijs (out for now; he/him)]

Yeah, we now always reappend this node, but we actually call updateAppMotif regularly, and should probably just check whether the existing motif is the same as the new one, in which case we shouldn't do anything (rather than reappending the node, which is probably causing blinking/weirdness if fetching the motif is expensive for some reason.

Comment 2

[Szabolcs Hubai (:xabolcs)]

It seems to me this is an easy fix.
In that case can I take it?

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Szabolcs Hubai (:xabolcs) from comment #2)
> It seems to me this is an easy fix.
> In that case can I take it?

Shouldn't be hard, no. Do check that doing /sync-whatever as a command still overrides this, in case you've changed the motif (but the URL has remained the same). :-)

Comment 4

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Szabolcs Hubai (:xabolcs) from comment #2)
> > It seems to me this is an easy fix.
> > In that case can I take it?
> 
> Shouldn't be hard, no. Do check that doing /sync-whatever as a command still
> overrides this, in case you've changed the motif (but the URL has remained
> the same). :-)

Well, assuming that worked before the thing that caused bug 888672; I actually guess layout probably optimizes for setting things to an identical value and, in that case, does nothing anyway. In which case we should possibly follow suit.

Comment 5

[Robert Utasi [:hunboy]]

I can repro this issue with this motif on official release of firefox, and on trunk also with fixed Bug 888864, but i can repro withOUT Bug 888672 patch also. Seems to me adding or removing reappend hack doesn't influence this.

Comment 6

[Szabolcs Hubai (:xabolcs)]

(In reply to :Gijs Kruitbosch from comment #4)
> Well, assuming that worked before the thing that caused bug 888672; I
> actually guess layout probably optimizes for setting things to an identical
> value and, in that case, does nothing anyway. In which case we should
> possibly follow suit.

I hope, I understand you correctly, because I was able to fix this bug with a one-liner. :)

Patch is coming soon.

Comment 7

[Szabolcs Hubai (:xabolcs)]

Attachment: patch v1 - do not reappend node blindly

Reappend node lazily.

Comment 8

[Szabolcs Hubai (:xabolcs)]

Attachment: chatzilla-0.9.90.1 with patch v1 applied

This is the patched (w/ patch v1) and repacked version of AMO version of ChatZilla.

Comment 9

[Thorbear]

Comment on attachment 786036 [details]
chatzilla-0.9.90.1 with patch v1 applied

That does indeed appear to fix the issue.

Comment 10

[:Gijs (out for now; he/him)]

Comment on attachment 786032 [details] [diff] [review]
patch v1 - do not reappend node blindly

Review of attachment 786032 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, thanks!

::: xul/content/static.js
@@ +2414,5 @@
>          {
>              node = document.createProcessingInstruction("xml-stylesheet", dataStr);
>              document.insertBefore(node, document.firstChild);
>          }
> +        else if (node.data !== dataStr)

Nit: no need for strict not-equals.

Comment 11

[Szabolcs Hubai (:xabolcs)]

Attachment: patch v1.1 - do not reappend node blindly

(In reply to :Gijs Kruitbosch from comment #10)
> Nit: no need for strict not-equals.

In this case, here is the updated patch.


PS: I'm not good at commit messages, feel free to reword it.

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 786161 [details] [diff] [review]
patch v1.1 - do not reappend node blindly

That's fine, I can fix the description on checkin, no worries. :-)

Comment 13

[Robert Utasi [:hunboy]]

Comment on attachment 786036 [details]
chatzilla-0.9.90.1 with patch v1 applied

Szabi, It's good, I removed ?feedback
Only 1 notice: the subject of patch "do not reappend node blindly"
can be "do not redefine stylesheet blindly" As we talked via email, it's independent from reappend issue based on my Comment 5.

Comment 14

[:Gijs (out for now; he/him)]

Moar patches!

Comment 15

[:Gijs (out for now; he/him)]

http://hg.mozilla.org/chatzilla/rev/c8a6adb73761