Open
Bug 1128353
Opened 9 years ago
Updated 7 years ago
[Enhancement] Make the messages layout use less tables
Categories
(Other Applications :: ChatZilla, enhancement)
Other Applications
ChatZilla
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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?
Assignee | ||
Comment 3•9 years ago
|
||
I consider the both changes backward compatible.
Assignee | ||
Updated•9 years ago
|
Attachment #8557668 -
Flags: review? → review?(bugzilla-mozilla-20000923)
Comment 4•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
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 :)
Updated•9 years ago
|
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Assignee | ||
Comment 6•9 years ago
|
||
(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)
Assignee | ||
Comment 7•9 years ago
|
||
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.
Comment 8•9 years ago
|
||
<!-- 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
Updated•9 years ago
|
Assignee: rginda → stoyan
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
Attachment #8557668 -
Attachment is obsolete: true
Attachment #8557668 -
Flags: review?(bugzilla-mozilla-20000923)
Comment 9•9 years ago
|
||
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.
Comment 10•9 years ago
|
||
(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.
Comment 11•9 years ago
|
||
a short testcase how flex works without tables: http://jsfiddle.net/qz9kjcuw/ opinion?
Assignee | ||
Comment 12•9 years ago
|
||
(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.
Comment 13•9 years ago
|
||
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.
Comment 14•9 years ago
|
||
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 15•9 years ago
|
||
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+
Comment 16•9 years ago
|
||
I'm not sure if we'd shipped new-style flexbox by esr31.
Comment 17•9 years ago
|
||
http://caniuse.com/flexbox says (click Show all) partial new-style was in 22 and full support was in 28.
Comment 18•9 years ago
|
||
Stoyan, here is a general way without javascript for splash: http://jsfiddle.net/qz9kjcuw/4/ feel free to think forward :)
Comment 19•9 years ago
|
||
a way to show/hide timestamp: http://jsfiddle.net/qz9kjcuw/5/ (css only)
Comment 20•9 years ago
|
||
(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.
Comment 21•9 years ago
|
||
(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?
Comment 22•9 years ago
|
||
(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. :)
Comment 23•9 years ago
|
||
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?
Comment 24•9 years ago
|
||
(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.
Comment 25•9 years ago
|
||
(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/
Assignee | ||
Comment 26•9 years ago
|
||
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)
Comment 27•9 years ago
|
||
Silver, There we go :) Collapsing consecutive nicks. fiddle here: http://jsfiddle.net/qz9kjcuw/10/
Comment 28•9 years ago
|
||
Silver, Reverse mode also works, Collapse/Separate Nicks fiddle here: http://jsfiddle.net/qz9kjcuw/12/
Comment 29•9 years ago
|
||
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)
Assignee | ||
Comment 30•9 years ago
|
||
(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)
Assignee | ||
Comment 31•9 years ago
|
||
(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.
Comment 32•9 years ago
|
||
(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).
Comment 33•9 years ago
|
||
(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.
Comment 34•9 years ago
|
||
new fiddle with "faces motif" : http://jsfiddle.net/qz9kjcuw/14/ I added some more style for better representation.
Comment 35•9 years ago
|
||
This one for that case when fiddle goes away for future usage.
Comment 36•9 years ago
|
||
[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
Comment 37•9 years ago
|
||
Comment 38•9 years ago
|
||
Comment 39•9 years ago
|
||
Comment 40•9 years ago
|
||
or something similar simplified code issue: inline-block doesn't support break-word
Comment 42•9 years ago
|
||
Gryllida, added hidden BR's to colwrapper and wrapped nicks to <> as a hidden way test it, pls, if it is what you want.
Assignee | ||
Comment 43•9 years ago
|
||
(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.
Comment 44•9 years ago
|
||
(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.
Comment 45•9 years ago
|
||
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 :)
Assignee | ||
Comment 46•9 years ago
|
||
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.
Assignee | ||
Comment 47•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8565866 -
Attachment description: 1128353-patch-2-splash-goes-flex.patch → 2. Splash now uses flex box layout
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Comment 48•9 years ago
|
||
Trim trailing whitespace of output-window.js
Attachment #8565867 -
Flags: review?(bugzilla-mozilla-20000923)
Assignee | ||
Comment 49•9 years ago
|
||
Trim trailing whitespace of commands.js
Attachment #8565871 -
Flags: review?(bugzilla-mozilla-20000923)
Assignee | ||
Comment 50•9 years ago
|
||
Trim trailing whitespace and fix newline chars of static.js
Attachment #8565883 -
Flags: review?(bugzilla-mozilla-20000923)
Assignee | ||
Comment 51•9 years ago
|
||
Simplify the networks list output markup. Use more of the internal functionality (newInlineText) to generate the output.
Attachment #8565891 -
Flags: review?(bugzilla-mozilla-20000923)
Assignee | ||
Updated•9 years ago
|
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 52•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8565867 -
Flags: review?(bugzilla-mozilla-20000923) → review+
Updated•8 years ago
|
Attachment #8565871 -
Flags: review?(bugzilla-mozilla-20000923) → review+
Updated•8 years ago
|
Attachment #8565883 -
Flags: review?(bugzilla-mozilla-20000923) → review+
Comment 53•8 years ago
|
||
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+
Assignee | ||
Comment 54•8 years ago
|
||
Reflects the review comments.
Attachment #8565866 -
Attachment is obsolete: true
Attachment #8713108 -
Flags: review?(bugzilla-mozilla-20000923)
Assignee | ||
Comment 55•8 years ago
|
||
Updated to the current content of the file.
Attachment #8565883 -
Attachment is obsolete: true
Attachment #8713110 -
Flags: review?(bugzilla-mozilla-20000923)
Updated•8 years ago
|
Attachment #8713110 -
Flags: review?(bugzilla-mozilla-20000923) → review+
Comment 56•8 years ago
|
||
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+
Comment 57•8 years ago
|
||
http://hg.mozilla.org/chatzilla/rev/efd2aebafb90 http://hg.mozilla.org/chatzilla/rev/7af8112972e6 http://hg.mozilla.org/chatzilla/rev/b2ca57398dd1 http://hg.mozilla.org/chatzilla/rev/59b2dbd0cf92 http://hg.mozilla.org/chatzilla/rev/07a9704b49b8 http://hg.mozilla.org/chatzilla/rev/18dd759c168d
Updated•8 years ago
|
Attachment #8557667 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8565867 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8557667 -
Attachment description: 1. Made indentations consistent → [checked in] 1. Made indentations consistent
Updated•8 years ago
|
Attachment #8565867 -
Attachment description: 3. Trim whitespace of output-window.js → [checked in] 3. Trim whitespace of output-window.js
Updated•8 years ago
|
Attachment #8565871 -
Attachment description: 4. Trim whitespace of commands.js → [checked in] 4. Trim whitespace of commands.js
Attachment #8565871 -
Attachment is obsolete: true
Updated•8 years ago
|
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
Updated•8 years ago
|
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
Updated•8 years ago
|
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
Assignee | ||
Comment 58•8 years ago
|
||
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 59•8 years ago
|
||
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?
Assignee | ||
Comment 60•8 years ago
|
||
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 61•8 years ago
|
||
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+
Comment 62•7 years ago
|
||
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)
Assignee | ||
Comment 63•7 years ago
|
||
Just to let you know that I'm on it, but it'll take me some time.
Comment 64•7 years ago
|
||
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.
Comment 65•7 years ago
|
||
And this one with checkbox toggle: https://jsfiddle.net/utasir/av2u85fr/6/
Comment 66•7 years ago
|
||
A small emulation to represent how it "works" : https://jsfiddle.net/utasir/av2u85fr/9/
Comment 67•7 years ago
|
||
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
Comment 68•7 years ago
|
||
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)
Assignee | ||
Comment 69•7 years ago
|
||
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 70•7 years ago
|
||
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.
Description
•