Open Bug 1128353 Opened 9 years ago Updated 7 years ago

[Enhancement] Make the messages layout use less tables

Categories

(Other Applications :: ChatZilla, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: stoyan, Assigned: stoyan)

Details

Attachments

(9 files, 13 obsolete files)

6.58 KB, text/html
Details
5.79 KB, text/html
Details
6.20 KB, text/html
Details
6.19 KB, text/html
Details
5.31 KB, text/html
Details
6.15 KB, text/html
Details
6.28 KB, text/html
Details
5.90 KB, text/html
Details
18.71 KB, patch
bugzilla-mozilla-20000923
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150129200438




Expected results:

This will be an attempt to convince the messages output and formatting mechanism of ChatZilla that using div-tags instead of table-tags is fun and in the end will make creating new motifs more easy for the front-end developers.
Normalizing the white spaces as the starting point. From the observation of the source file (output-window.html) I was concluding that indentation of 2 spaces per tab is used. The trailing spaces were also removed.
Attachment #8557667 - Flags: review?(bugzilla-mozilla-20000923)
I personally think that this kind of styling should be left for the motif to handle, but giving the interactive element (onResize) the simplified code will provide a good start in this direction.
Attachment #8557668 - Flags: review?
I consider the both changes backward compatible.
Attachment #8557668 - Flags: review? → review?(bugzilla-mozilla-20000923)
Comment on attachment 8557668 [details] [diff] [review]
Made splash as higher, as intended

> function onResize()
> {
>-    var halfHeight = Math.floor(window.innerHeight / 2);
>-    var splash = document.getElementById("splash");
>-    splash.style.paddingTop = halfHeight + "px";
>-    splash.style.paddingBottom = halfHeight + "px";
>+    splash.style.lineHeight = window.innerHeight + "px";
> }

stoyan, you removed the node also, so splash is null now.
Flags: needinfo?(stoyan)
stoyan,

http://www.magyarmenedek.com/vegyes/ww4.html

I was just playing with it in the past, and a small comment: how about long text in splash?

anyway flex will be the answer of this problem, when the backwards compatibility goes above and reaches flex-compatible Gecko. until that we should play with javascript and tables :)
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
(In reply to Robert Utasi [:hunboy] from comment #4)
> Comment on attachment 8557668 [details] [diff] [review]
> Made splash as higher, as intended
> 
> > function onResize()
> > {
> >-    var halfHeight = Math.floor(window.innerHeight / 2);
> >-    var splash = document.getElementById("splash");
> >-    splash.style.paddingTop = halfHeight + "px";
> >-    splash.style.paddingBottom = halfHeight + "px";
> >+    splash.style.lineHeight = window.innerHeight + "px";
> > }
> 
> stoyan, you removed the node also, so splash is null now.

splash 'leaks in' from row 122 where defined in the wrapping function.

Nonetheless, your previous comment is valid and the patch shouldn't be accepted as such.

What is the BC policy? Is there as certain number of versions to keep BC or is per usage statistics?
Flags: needinfo?(stoyan)
Robert, how flex should help here? The behaviour of the splash is to resize itself to the window size. If I'm not missing something flex math is based on the document geometry.
    <!-- Firefox
         3.5 is Gecko 1.9.1
         3.6 is Gecko 1.9.2
         4.0 is Gecko 2.0
         5.0 is Gecko 5.0
         x.0 is Gecko x.0, where x is greater than 5 -->
    <em:targetApplication>
      <Description>
        <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
        <em:minVersion>3.5</em:minVersion>
        <em:maxVersion>35.*</em:maxVersion>
      </Description>
    </em:targetApplication>
 Here is the current BComp definition, you always can find it in the chatzilla.xpi as install.rdf
Assignee: rginda → stoyan
Status: NEW → ASSIGNED
Attachment #8557668 - Attachment is obsolete: true
Attachment #8557668 - Flags: review?(bugzilla-mozilla-20000923)
Stoyan,

we have a small problem at Bug 587272 . The problem is: word-wrap has no effect in table cells, so the only way is using DIVisions with flex:auto. So when you reduce or eliminate the tablestructure, it is a viewpoint. (currently not possible by BComp)

BTW the tablestructure has a nested table which one was a workaround for a bug existed in the Netscape times, but not now.

And there are some ECMA standard functions such as insertRow() insertCell() for the tables but applying them we need be sure if they are in a correct namespace. (these functions are absolutely backwards compatible)

you should find me anytime on chat under username hunboy.
(In reply to Robert Utasi [:hunboy] from comment #8)
>         <em:minVersion>3.5</em:minVersion>
>         <em:maxVersion>35.*</em:maxVersion>

For what it's worth, I'm pretty tempted to bump the minimum version up to the most recent ESR (currently 31) unless anyone has specific issues with that.
a short testcase how flex works without tables: http://jsfiddle.net/qz9kjcuw/

opinion?
(In reply to Robert Utasi [:hunboy] from comment #5)
> anyway flex will be the answer of this problem, when the backwards
> compatibility goes above and reaches flex-compatible Gecko. until that we
> should play with javascript and tables :)
I have probably misunderstood you, because I was thinking the comment is related to the splash issue, not in general.
Stoyan, Silver

another short testcase with scaled width: http://jsfiddle.net/qz9kjcuw/2/

opinion?

we just need to find a way for "collapse consecutive messages" function for it.

Stoyan, it was a general concept.
One of the problems of separate layout for each row - whether using flex or anything not tables - is that the timestamp "column" must be fixed-width, but the timestamp text can (and does) vary in length. We'll need to be careful about how this is set up. :)

flex can center things in both axes so it may be possible to do the splash with it, but I haven't checked.
Comment on attachment 8557667 [details] [diff] [review]
[checked in] 1. Made indentations consistent

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

Looks fine, r=silver
Attachment #8557667 - Flags: review?(bugzilla-mozilla-20000923) → review+
I'm not sure if we'd shipped new-style flexbox by esr31.
http://caniuse.com/flexbox says (click Show all) partial new-style was in 22 and full support was in 28.
Stoyan,

here is a general way without javascript for splash: http://jsfiddle.net/qz9kjcuw/4/

feel free to think forward :)
a way to show/hide timestamp: http://jsfiddle.net/qz9kjcuw/5/
(css only)
(In reply to James Ross from comment #14)
> One of the problems of separate layout for each row - whether using flex or
> anything not tables - is that the timestamp "column" must be fixed-width,
> but the timestamp text can (and does) vary in length. We'll need to be
> careful about how this is set up. :)

Well, yes, if the font is not equal width this should go to ugly, workaround is using monospace font for the timestamp. At the current testcase it is zero length, so fits to timestamp as long defined by user in the preferences. Normally the numbers are equal width in the most used fonts so this looks as a real table. However if we define min-width for this field that must depend on the user defined format.
(In reply to Robert Utasi [:hunboy] from comment #20)
> (In reply to James Ross from comment #14)
> > One of the problems of separate layout for each row - whether using flex or
> > anything not tables - is that the timestamp "column" must be fixed-width,
> > but the timestamp text can (and does) vary in length. We'll need to be
> > careful about how this is set up. :)
> 
> Well, yes, if the font is not equal width this should go to ugly, workaround
> is using monospace font for the timestamp. At the current testcase it is
> zero length, so fits to timestamp as long defined by user in the
> preferences. Normally the numbers are equal width in the most used fonts so
> this looks as a real table. However if we define min-width for this field
> that must depend on the user defined format.

I'd really prefer not to force everyone to use monospace fonts here just because it makes the layout story easier. Can't we just set it to a fixed number of em-s based on the format?
(In reply to :Gijs Kruitbosch from comment #21)
> I'd really prefer not to force everyone to use monospace fonts here just
> because it makes the layout story easier. Can't we just set it to a fixed
> number of em-s based on the format?

We're going to have to either: guestimate the size, or let the user set it. Requiring monospace fonts is a non-option. :)
another issue with fixed width: what happens when we change the font size in pt? Or the font itself? (we are thinking in general flowed text assumed the timestamp doesn't behave as monospace)

normally we can do implicit maximum search during inserting lines, and if the latest line's width is bigger than the previous ones, we can do insertRule() for the class with a new min-width. This is fast, realtime, etc.
Problems:
1. see above
2. what happens during using /sync-output? do we use the actual maxsize?
3. what happens when the widest line flushes away?
4. getComputedStyle() always use px value instead of em or pt.

Anyhow this is cheaper as layout but expensiver as workarounds to simulate table behavior. Excet if we adopt that concept if numbersize is fixed in the most used fonts.

And 1 question is still left: how we do "Collapse consecutive messages" function?
(In reply to Robert Utasi [:hunboy] from comment #13)
> another short testcase with scaled width: http://jsfiddle.net/qz9kjcuw/2/
> 
> opinion?

When I try to copy the lines, each line copies as 3 lines (ts, nick, message on separate lines). I haven't checked whether this is the case with the latest example in this discussion, but for usability, I think, one IRC line should copy as 1 line with nickname enclosed in <>s.
(In reply to Gryllida from comment #24)
> When I try to copy the lines, each line copies as 3 lines (ts, nick, message
> on separate lines). I haven't checked whether this is the case with the
> latest example in this discussion, but for usability, I think, one IRC line
> should copy as 1 line with nickname enclosed in <>s.

Gryllida,

We should replace divs with spans for one-line copy, but in this case we need add whitespace textNodes between spans and works ok. Your "<" and ">" issue is a style issue, these marks come from the motif as pseudo-elements, so paste & copy doesn't copy this. the only way is adding to the code permanently as textNode.

Silver,

I implemented a way how to make equal width for all columns currently for the .nick column because it's more visible than timestamp fields if good or not. This is an explicit way whilst this is a fiddle only, not realtime, not an implicit realtime stuff.

new fiddle here: http://jsfiddle.net/qz9kjcuw/8/
Made output-window.html html5. Remove |onResize()| and commented/obsolete CSS rules.

Thanks to Robert for the pointers and the code. All credits for the CSS goes to him.

This patch depends on "Made indentations consistent" one. If it is not good to be like this, please, tell me.
Attachment #8558455 - Flags: ui-review?
Attachment #8558455 - Flags: review?(bugzilla-mozilla-20000923)
Silver,

There we go :)

Collapsing consecutive nicks.

fiddle here: http://jsfiddle.net/qz9kjcuw/10/
Silver,

Reverse mode also works, Collapse/Separate Nicks

fiddle here: http://jsfiddle.net/qz9kjcuw/12/
Comment on Attachment 8558455 [details] [diff]

Stoyan, Silver,

we use const XHTML_NS = "http://www.w3.org/1999/xhtml";
to create elements under namespace with createElementNS in xul for xhtml.

using short html5 doctype doesn't disturb it?

see here: http://www.w3.org/TR/xhtml2/conformance.html

or this one we need? :

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 2.0//EN"
    "http://www.w3.org/MarkUp/DTD/xhtml2.dtd">
Flags: needinfo?(stoyan)
(In reply to Robert Utasi [:hunboy] from comment #29)
> Comment on Attachment 8558455 [details] [diff]
> 
> Stoyan, Silver,
> 
> we use const XHTML_NS = "http://www.w3.org/1999/xhtml";
> to create elements under namespace with createElementNS in xul for xhtml.

The presence of NS is required in order to allow us retrieving and properly render HTML tags in XUL document. There are two ways of supplying it. The first is by adding one to the <html> element in the target document (i.e. output-window.html) and the second is by providing one as an attribute to the <window> element of the XUL document. Currently we have NS as attribute of the <window> of chatzilla.xul so that we can safely refer to it with the constant you mention.
Flags: needinfo?(stoyan)
(In reply to Gryllida from comment #24)
> When I try to copy the lines, each line copies as 3 lines (ts, nick, message
> on separate lines). I haven't checked whether this is the case with the
> latest example in this discussion, but for usability, I think, one IRC line
> should copy as 1 line with nickname enclosed in <>s.

Altering the layout - the internal structure of the presented information it is possible to break this behaviour. The real question is to what extend we should stick to it? In this case it is required to keep the plain text structure intact since it is used by every body, probably every day. But, for example, the available networks list if migrated to ul/li-s will improve the semantics and accessibility, but break the old way it worked.
(In reply to Стоян Димитров [:stoyan] from comment #31)
> structure intact since it is used by every body, probably every day. But,
> for example, the available networks list if migrated to ul/li-s will improve
> the semantics and accessibility, but break the old way it worked.

Bear in mind that, although we can display any HTML in the output window, it all has to convert well to plain text for logging (in addition to copying well).
(In reply to Стоян Димитров [:stoyan] from comment #30)
> (In reply to Robert Utasi [:hunboy] from comment #29)
> > Comment on Attachment 8558455 [details] [diff]
> > 
> > Stoyan, Silver,
> > 
> > we use const XHTML_NS = "http://www.w3.org/1999/xhtml";
> > to create elements under namespace with createElementNS in xul for xhtml.
> 
> The presence of NS is required in order to allow us retrieving and properly
> render HTML tags in XUL document. There are two ways of supplying it. The
> first is by adding one to the <html> element in the target document (i.e.
> output-window.html) and the second is by providing one as an attribute to
> the <window> element of the XUL document. Currently we have NS as attribute
> of the <window> of chatzilla.xul so that we can safely refer to it with the
> constant you mention.

Stoyan,

my question is: we use xhtml instead of html, so we probably need xhtml doctype instead of short html doctype for xhtml2 which one equals to html5.
new fiddle with "faces motif" : http://jsfiddle.net/qz9kjcuw/14/

I added some more style for better representation.
This one for that case when fiddle goes away for future usage.
[21:30]	<hunboy>	Silver, does this idea worth any effort nowadays to think forward? : https://bug1128353.bugzilla.mozilla.org/attachment.cgi?id=8559227 or just using display: table-row|table-cell|table on divs/spans nowadays?
[22:06]	<Silver>	hunboy: I think it's worth exploring, although I'm not sure I like the amount of JS and manipulation it uses for the collapse nicks toggle.
[22:11]	<hunboy>	Silver, It's not more hardcoded than the current collapsing algorithm. Probably looks lots because it's an explicit testcase, not an implicit one at __display() My current problem is the timestampfield. How to modellize the different timestamps as a masonry layout...
[22:13]	<Silver>	Perhaps.
[22:13]	<Silver>	I'd like to keep exploring ideas for now before we settle on any particular way of doing it.
[22:21]	<hunboy>	Silver, In my opinion, I'm not fond of table visibility at any rate. Perhaps I can imagine some type of flowing as well. There is a claim for mobile usage as well, and that requires 100% flowing of the texts.
[22:21]	<Silver>	hunboy: As in, flowing the nickname and chat as one block?
[22:22]	<hunboy>	Yup. perhaps.
[22:22]	* Silver	actually uses timestamps, nicks and chat in a table layout on his phone fine.
[22:22]	<Silver>	But I do use an annoyingly small font for other people. :)
[22:22]	<hunboy>	:D
[23:24]	<hunboy>	Silver, I can imagine this type of layout, too: http://fiddle.jshell.net/qz9kjcuw/17/show/ not sure if we need space before the short nicks.
[00:06]	<Silver>	Yeah I really don't like that, it's just impossible to read. :)
[00:20]	<hunboy>	We'we just used to the table layout. :) all of messengers work this way I know.
[00:22]	<hunboy>	As a compromise we should add percentage flex value and some media-query not to look ugly in wide screen :)
[00:23]	<hunboy>	Silver, here is a sample: http://fiddle.jshell.net/qz9kjcuw/19/show/ just resize the window :)
[00:27]	<hunboy>	However I only modified the css, so it is a motif issue :)
[01:10]	<hunboy>	Silver, http://fiddle.jshell.net/qz9kjcuw/20/show/ full table layout overdefined at the end of the css with display: table|table-row|table-cell
[01:11]	<hunboy>	So this is also only css with same html structure.
[01:16]	<hunboy>	I think - if we find any way to make masonry for timestamps vertically - this can be the new structure, everything else is only motif question. Perhaps we define 2x3 motifs 2table, 2half-table and 2fullflex and user can decide what he wants from the 3 possibility in light and or dark
[01:23]	<hunboy>	Some fix in timestamp-checkbox http://fiddle.jshell.net/qz9kjcuw/21/show/ but this fix is only css, so read above ^ :)
[01:52]	<hunboy>	Ok, now pixelperfect: http://fiddle.jshell.net/qz9kjcuw/22/show/ I forgot to remove the margin-left/right 9_9
or something similar simplified code
issue: inline-block doesn't support break-word
This one better
Attachment #8559978 - Attachment is obsolete: true
Attached file Screen Copy Testcase
Gryllida,

added hidden BR's to colwrapper
and wrapped nicks to <> as a hidden way

test it, pls, if it is what you want.
(In reply to Robert Utasi [:hunboy] from comment #33)
 
> my question is: we use xhtml instead of html, so we probably need xhtml
> doctype instead of short html doctype for xhtml2 which one equals to html5.

Good point! We must use XHTML instead of HTML.

(In reply to James Ross from comment #32)
> Bear in mind that, although we can display any HTML in the output window, it
> all has to convert well to plain text for logging (in addition to copying
> well).
Those are three representations of one data that should be handled separately. They serve different purposes and are consumed differently. What control we have over the text user copies? This is the big question here and of its answer depends our the freedom to change the layout.
(In reply to Стоян Димитров [:stoyan] from comment #43)
> Those are three representations of one data that should be handled
> separately. They serve different purposes and are consumed differently. What
> control we have over the text user copies? This is the big question here and
> of its answer depends our the freedom to change the layout.

We currently have no control over logging anything other than what's extracted from the HTML (IIRC) or for what's copied by a user. Both could be overcome but neither are easy and I would recommend not making the work in this bug depend on either being changed.
Stojan,

Just upload your ideas as testcase, we are gathering now ideas what to do, and Silver will pick one, or we will make a merged version based on experiences and ideas.

The layout building is at __display() and addHistory() functions in the static.js file. The current concept is: the js part can't be more hardcoded than the current one. But some "wizzards" should be accepted :)
Robert, I don't see a point as per comment 44. All the front-end work must be compromised to workaround the limitations described there (i.e. use <span> instead of <div>, inability to use semantic structures).
Instead of layout rework I will try to simplify the generated DOM structures from the code while keeping the current functionality intact. A small but needed step.
Revert back to XHTML doctype, add missing xmlns attrib to <html>
Attachment #8558455 - Attachment is obsolete: true
Attachment #8558455 - Flags: ui-review?
Attachment #8558455 - Flags: review?(bugzilla-mozilla-20000923)
Attachment #8565866 - Flags: review?(bugzilla-mozilla-20000923)
Attachment #8565866 - Attachment description: 1128353-patch-2-splash-goes-flex.patch → 2. Splash now uses flex box layout
Attachment #8557667 - Attachment description: Made indentations consistent → 1. Made indentations consistent
Attachment #8557667 - Attachment filename: output-window.html.patch → 1128353-patch-1-output-window-indentation.patch
Trim trailing whitespace of output-window.js
Attachment #8565867 - Flags: review?(bugzilla-mozilla-20000923)
Trim trailing whitespace of commands.js
Attachment #8565871 - Flags: review?(bugzilla-mozilla-20000923)
Trim trailing whitespace and fix newline chars of static.js
Attachment #8565883 - Flags: review?(bugzilla-mozilla-20000923)
Simplify the networks list output markup. Use more of the internal functionality (newInlineText) to generate the output.
Attachment #8565891 - Flags: review?(bugzilla-mozilla-20000923)
Attachment #8565883 - Attachment description: 5 Trim whitespace and fix newline chars of static.js → 5. Trim whitespace and fix newline chars of static.js
Comment on attachment 8565866 [details] [diff] [review]
2. Splash now uses flex box layout

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

Overall, this looks like a good change. There's a couple of things I think we can improve further.

Apologies for the delay in reviewing this.

::: xul/content/output-base.css
@@ +127,5 @@
>  
> +#splash-wrapper {
> +    display: flex;
> +    height: 100vh;
> +    flex-flow: row nowrap;

These are the initial values so this can be removed, but I don't object to it being kept just in case.

::: xul/content/output-window.html
@@ +5,2 @@
>  <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
>    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

Since we here, let's simplify this to <!DOCTYPE html>.

@@ +5,4 @@
>  <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
>    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
>  
> +<html xmlns="http://www.w3.org/1999/xhtml">

I don't think this is needed.

@@ +9,2 @@
>    <head>
> +    <meta http-equiv="content-type" content="text/html; charset=utf-8" />

Similarly, this should be fine with just <meta charset="utf-8"> here.
Attachment #8565866 - Flags: review?(bugzilla-mozilla-20000923) → review+
Attachment #8565867 - Flags: review?(bugzilla-mozilla-20000923) → review+
Attachment #8565871 - Flags: review?(bugzilla-mozilla-20000923) → review+
Attachment #8565883 - Flags: review?(bugzilla-mozilla-20000923) → review+
Comment on attachment 8565891 [details] [diff] [review]
[checked in] 6. Simplified the networks list output markup

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

Good use of newInlineText. :)
Attachment #8565891 - Flags: review?(bugzilla-mozilla-20000923) → review+
Reflects the review comments.
Attachment #8565866 - Attachment is obsolete: true
Attachment #8713108 - Flags: review?(bugzilla-mozilla-20000923)
Updated to the current content of the file.
Attachment #8565883 - Attachment is obsolete: true
Attachment #8713110 - Flags: review?(bugzilla-mozilla-20000923)
Attachment #8713110 - Flags: review?(bugzilla-mozilla-20000923) → review+
Comment on attachment 8713108 [details] [diff] [review]
[checked in] 2. Splash now uses flex box layout

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

r=silver based on the diff to previous version of patch

Thanks for the work, we'll try and get this committed soon (I'm currently away for a few days).
Attachment #8713108 - Flags: review?(bugzilla-mozilla-20000923) → review+
Attachment #8557667 - Attachment is obsolete: true
Attachment #8565867 - Attachment is obsolete: true
Attachment #8557667 - Attachment description: 1. Made indentations consistent → [checked in] 1. Made indentations consistent
Attachment #8565867 - Attachment description: 3. Trim whitespace of output-window.js → [checked in] 3. Trim whitespace of output-window.js
Attachment #8565871 - Attachment description: 4. Trim whitespace of commands.js → [checked in] 4. Trim whitespace of commands.js
Attachment #8565871 - Attachment is obsolete: true
Attachment #8565891 - Attachment description: 6. Simplified the networks list output markup → [checked in] 6. Simplified the networks list output markup
Attachment #8565891 - Attachment is obsolete: true
Attachment #8713108 - Attachment description: 2. Splash now uses flex box layout → [checked in] 2. Splash now uses flex box layout
Attachment #8713108 - Attachment is obsolete: true
Attachment #8713110 - Attachment description: 5. Trim whitespace and fix newline chars of static.js → [checked in] 5. Trim whitespace and fix newline chars of static.js
Attachment #8713110 - Attachment is obsolete: true
This patch removes the "chatzilla-link" class from CSS files and the rest of the code also fixes few broken new-line characters in "mungers.js" . The presentation of the links is kept as before.
Attachment #8772432 - Flags: review?(bugzilla-mozilla-20000923)
Comment on attachment 8772432 [details] [diff] [review]
Remove unneeded anchor class name

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

::: xul/content/output-base.css
@@ +148,1 @@
>  	color: #0000EE;

The indenting here is wonky, and this should keep a:link so it doesn't apply to anchor tags.

::: xul/content/output-window.html
@@ +27,4 @@
>          border: 1px black solid;
>        }
>  
> +      table,

Did you test this with nested message tables?
This patch removes the "chatzilla-link" class from CSS files and the rest of the code also fixes few broken new-line characters in mungers.js. The presentation of the links is kept as before. Also fixes some indentation inconsistencies and trailing whitespaces.

Reverted the part that touches tables.
Attachment #8772432 - Attachment is obsolete: true
Attachment #8772432 - Flags: review?(bugzilla-mozilla-20000923)
Attachment #8772775 - Flags: review?(bugzilla-mozilla-20000923)
Comment on attachment 8772775 [details] [diff] [review]
Remove unneeded anchor class name

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

Sorry this took a while, I've been very busy but finally working through the backlog.

This patch looks good, with just a couple of small potential issues which you can address either with an updated version of this patch, or a subsequent patch, whichever is easier for you.

::: xul/content/output-base.css
@@ -154,5 @@
>  }
> -
> -/* links */
> -a.chatzilla-link {
> -    text-decoration: none;

If a link is visited, only :visited matches, so the text-decoration in a:link won't apply. We should remove the underline for both :link and :visited.

@@ -156,5 @@
> -/* links */
> -a.chatzilla-link {
> -    text-decoration: none;
> -    direction: ltr;
> -    unicode-bidi: embed;

We might want to keep these two lines, otherwise I think links in RTL display will be messed up. Feel free to confirm/deny with an actual test after this patch is committed.
Attachment #8772775 - Flags: review?(bugzilla-mozilla-20000923) → review+
Stoyan:
Is this patch (after addressing the few nits in comment #61) all that is needed to fix the problem?

If the answer is "yes", you should attach an updated (and, if necessary, un-bitrotted) patch, carry over r+ set in comment #61, and set the "checkin-needed" keyword.
Flags: needinfo?(stoyan)
Just to let you know that I'm on it, but it'll take me some time.
In FireFox 51 the grid layout has been introducing, on chat we prepared a very simplified testcase to represent it:

https://jsfiddle.net/utasir/av2u85fr/5/

Not sure if this will be the way, but good to know if this one also available.
And this one with checkbox toggle: https://jsfiddle.net/utasir/av2u85fr/6/
A small emulation to represent how it "works" :

https://jsfiddle.net/utasir/av2u85fr/9/
I upload here the small emulation for those users who can't access the jsfiddle by policy. Note that in ff51 grid must be enabled in about:config
There is possible with grid-auto-flow: row dense; to completely reorder the sctructure. At this sample we can toggle the date column to right. column-start redefinitions in css :) Toggle button at top-right-corner of screen 

(In a similar way we can move the nick fields aswell to right for rtl users if necessary, but that is not in this sample.)

(Note, this is still a simplified emulation)
Updated the anchors section at the top of output-base.css according to the reviewer comments.
Attachment #8772775 - Attachment is obsolete: true
Flags: needinfo?(stoyan)
Attachment #8854436 - Flags: review?(bugzilla-mozilla-20000923)
Comment on attachment 8854436 [details] [diff] [review]
Remove unneeded anchor class name (updated)

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

Looks good, thanks!
Attachment #8854436 - Flags: review?(bugzilla-mozilla-20000923) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: