Last Comment Bug 649266 - Certificate window is broken with long text labels
: Certificate window is broken with long text labels
Status: VERIFIED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Security: UI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Francesco Lodolo [:flod]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-12 02:15 PDT by Francesco Lodolo [:flod]
Modified: 2011-08-19 23:39 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Broken certificate window (35.66 KB, image/png)
2011-04-12 02:15 PDT, Francesco Lodolo [:flod]
no flags Details
Fix to make long text wrap (1.23 KB, patch)
2011-06-04 22:54 PDT, Francesco Lodolo [:flod]
ludovic: review-
Details | Diff | Splinter Review
Certificate window with the fix (41.09 KB, image/png)
2011-06-04 22:55 PDT, Francesco Lodolo [:flod]
no flags Details
Fix to make long text wrap using textContent (1.21 KB, patch)
2011-06-20 11:12 PDT, Francesco Lodolo [:flod]
kaie: review+
Details | Diff | Splinter Review

Description Francesco Lodolo [:flod] 2011-04-12 02:15:21 PDT
Created attachment 525337 [details]
Broken certificate window

This is happening with the Italian build of Firefox 4 (verified on Mac and Windows): if the first label is too long, the text doesn't wrap and the controls are displayed partially (see also the almost missing button in the bottom right corner).

Another issue is that this windows can't be resized.

From a quick check on mxr.mozilla.org, some other locales should have the same problem (e.g. sl, pt-BR, gd).
Comment 1 Francesco Lodolo [:flod] 2011-06-04 22:54:15 PDT
Created attachment 537422 [details] [diff] [review]
Fix to make long text wrap

Since nobody is working on this and localized builds are still broken, I tried the DIY way. I'm not a programmer, so I need some help to understand if I'm doing things right.

My understanding after browsing MDC for a couple of hours is that text set as value in a label doesn't wrap, while it wraps if set as a text child node.

In this case I think that adding a child node without checking if there are already other nodes is fine.
Comment 2 Francesco Lodolo [:flod] 2011-06-04 22:55:45 PDT
Created attachment 537423 [details]
Certificate window with the fix

Certificate window with attached patch applied
Comment 3 Francesco Lodolo [:flod] 2011-06-08 00:52:27 PDT
I forgot one thing: if this works and it can be considered a low risk change, it would be great to land it also on other branches, not only on mozilla-central.

Other languages are probably at the limit (for example sl), some (pt-BR, gd) have strings longer than Italian.
Comment 4 Kai Engert (:kaie) 2011-06-08 03:35:44 PDT
I would expect that you remove unnecessary xul elements.
With your patch, the <label class="header" id="verified"/> is no longer used.

I understand you use the "verified" node as the insertion point.
Can you use a neighbourhood node as the insertion point, and remove the verified element?
Comment 5 Francesco Lodolo [:flod] 2011-06-08 04:05:43 PDT
(In reply to comment #4)
> I would expect that you remove unnecessary xul elements.
> With your patch, the <label class="header" id="verified"/> is no longer used.

The label is used as parent of the text node (and AFAIK that's the only way to make long text wraps). My understanding is that, if I remove that element from the XUL file, I should still create it at run-time and this wouldn't be a better solution. Am I missing something?
Comment 6 Francesco Lodolo [:flod] 2011-06-09 00:23:09 PDT
I made a couple of tests. If I attach the text node to the parent vbox, text is displayed differently (normal vs bold) and in the same way of usages.

The only solution I found to display the text in the same way is to recreate the same XUL structure (text node appended to a label with a class="header"). If you think that's the best solution I can provide an updated patch.
Comment 7 Francesco Lodolo [:flod] 2011-06-20 04:19:48 PDT
Any chance to make this bug progress? I'd really like to see this issue fixed and I'm open to suggestions ;-)
Comment 8 Philip Chee 2011-06-20 09:02:19 PDT
> -  verified.setAttribute("value", verifystr);
> +  verified.appendChild(document.createTextNode(verifystr));

Can you try this instead:
verified.textContent = verifystr;
Comment 9 Francesco Lodolo [:flod] 2011-06-20 11:11:46 PDT
Yep, .textContent works fine (text wraps and is displayed with bold font). Attaching a new patch in case that's a good solution to speed things up.
Comment 10 Francesco Lodolo [:flod] 2011-06-20 11:12:39 PDT
Created attachment 540539 [details] [diff] [review]
Fix to make long text wrap using textContent
Comment 11 Francesco Lodolo [:flod] 2011-07-17 22:08:50 PDT
Sorry for the bugspam, but is there any chance to get the last patch reviewed?
Comment 12 Kai Engert (:kaie) 2011-07-25 02:41:46 PDT
Comment on attachment 540539 [details] [diff] [review]
Fix to make long text wrap using textContent

I don't know XUL well enough to say if this code is correct. 

If you are confident this works cross platform, r=kaie
Comment 13 Francesco Lodolo [:flod] 2011-07-25 02:52:40 PDT
Thanks Kai, I don't think this kind of property is platform specific. Anyway I'll test all platforms as soon as the fix land on mozilla-central.
Comment 14 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-18 03:00:57 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/acf10d96fa73
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-19 03:06:02 PDT
http://hg.mozilla.org/mozilla-central/rev/acf10d96fa73
Comment 16 Francesco Lodolo [:flod] 2011-08-19 23:39:26 PDT
Verified on Gecko/20110819 Firefox/9.0a1 (Snow Leopard, Ubuntu and Win7)

Note You need to log in before you can comment on or make changes to this bug.