Hi Tim, On Wed, Dec 09, 2009 at 12:54:54AM +0100, Tim Nieradzik wrote:
In my opinion, these changes make sense indeed as they provide a more generic interface.
Looking at quirkmode's "display: list-item"-demo, listStylePosition seems to have a wrong value. The bullets are supposed to be drawn outside the box but they appear inside. Did I understand your changes correctly that listStylePosition is either 0 ("outside") or 1 ("inside") because of that enum?
I guess you're right, that the bullets should be outside the box, but due to the current implementation of ListItem it's inside even if listStylePosition is outside. However as long as you don't look at the border of the box, the behaviour is correct now, see e.g. http://de.selfhtml.org/css/eigenschaften/anzeige/list_style_position.htm
I tried to implement "display: none" but the contents are still visible. I also worked on getting rid of tagIsDisplayEnabled but I am yet uncertain whether it is safe how approached it. Any comments?
Just yesterday, I added the DISPLAY_NONE to dillo mainline, just as you did here :-) I'm still uncertain on what to do in case that DISPLAY_NONE is set. As you noticed it's not enough to not call the tag open functions, as text will still be added. Also I don't know, whether e.g. inputs in forms which have DISPLAY_NONE, should send their contents to the server in case of submit, or should they be just non-existent? Regarding tagIsDisplayEnabled, I don't quite follow what you intended? I had added this flag to gradually transform the Html_tag_open_*() functions to the new display handling. It can only be removed once all these functions have been converted. Hm, maybe you are right and as long as the corresponding tags are set to DISPLAY_INLINE, which is the default, it would work even without the tagIsDisplayEnabled flag. Anyway, I think it's still a good marker to see which functions have been converted. Regards, Johannes
--Tim
diff -r 440fa7e952bd dw/style.hh --- a/dw/style.hh Tue Dec 08 18:46:31 2009 +0100 +++ b/dw/style.hh Wed Dec 09 00:48:03 2009 +0100 @@ -253,7 +253,8 @@ DISPLAY_TABLE_FOOTER_GROUP, DISPLAY_TABLE_ROW, DISPLAY_TABLE_CELL, - DISPLAY_LAST + DISPLAY_LAST, + DISPLAY_NONE };
enum ListStylePosition { diff -r 440fa7e952bd src/cssparser.cc --- a/src/cssparser.cc Tue Dec 08 18:46:31 2009 +0100 +++ b/src/cssparser.cc Wed Dec 09 00:48:03 2009 +0100 @@ -58,10 +58,10 @@ "w-resize", "text", "wait", "help", NULL };
-static const char *const Css_display_enum_vals[DISPLAY_LAST + 1] = { +static const char *const Css_display_enum_vals[] = { "block", "inline", "list-item", "table", "table-row-group", "table-header-group", "table-footer-group", "table-row", - "table-cell", NULL + "table-cell", "last", "none", NULL };
static const char *const Css_font_size_enum_vals[] = { diff -r 440fa7e952bd src/html.cc --- a/src/html.cc Tue Dec 08 18:46:31 2009 +0100 +++ b/src/html.cc Wed Dec 09 00:48:03 2009 +0100 @@ -1282,7 +1282,7 @@ TagInfo toptag = Tags[toptag_idx]; if (s_sz > idx + 1 && toptag.EndTag != 'O') BUG_MSG(" - forcing close of open tag: <%s>\n", toptag.name); - _MSG("Close: %*s%s\n", size," ", toptag.name); + _MSG("Close: %*s%s\n", s_sz, " ", toptag.name); toptag.close(html, toptag_idx); Html_real_pop_tag(html); } @@ -1528,13 +1528,6 @@ }
/* - * By setting this to true, a Html_tag_open* function indicates that - * the common display handling code should be used. - * TODO: convert all Html_tag_open* functions and remove this variable. - */ -static bool tagIsDisplayEnabled; - -/* * Handle open HTML element */ static void Html_tag_open_html(DilloHtml *html, const char *tag, int tagsize) @@ -1876,8 +1869,6 @@ a_Html_stash_init(html); S_TOP(html)->parse_mode = DILLO_HTML_PARSE_MODE_STASH_AND_BODY; - - tagIsDisplayEnabled = true; }
/* @@ -1916,8 +1907,6 @@
html->styleEngine->setNonCssHints (&props); dFree(fontFamily); - - tagIsDisplayEnabled = true; }
/* @@ -1926,7 +1915,7 @@ static void Html_tag_open_abbr(DilloHtml *html, const char *tag, int tagsize) { const char *attrbuf; - + if (prefs.show_tooltip && (attrbuf = a_Html_get_attr(html, tag, tagsize, "title"))) { CssPropertyList props; @@ -1936,8 +1925,6 @@ html->styleEngine->setNonCssHints (&props); dFree(tooltip_str); } - - tagIsDisplayEnabled = true; }
/* @@ -2183,8 +2170,6 @@ BUG_MSG("name attribute is required for <map>\n"); } } - - tagIsDisplayEnabled = true; }
/* @@ -2453,8 +2438,6 @@ dFree(nameVal); } } - - tagIsDisplayEnabled = true; }
/* @@ -2528,8 +2511,6 @@ S_TOP(html)->list_type = HTML_LIST_UNORDERED; S_TOP(html)->list_number = 0; S_TOP(html)->ref_list_item = NULL; - - tagIsDisplayEnabled = true; }
/* @@ -2590,10 +2571,9 @@ BUG_MSG( "illegal '-' character in START attribute; Starting from 0\n"); n = 0; } + S_TOP(html)->list_number = n; S_TOP(html)->ref_list_item = NULL; - - tagIsDisplayEnabled = true; }
/* @@ -2621,8 +2601,6 @@ *list_number = 0; } } - - tagIsDisplayEnabled = true; }
/* @@ -3011,7 +2989,6 @@ static void Html_tag_open_default(DilloHtml *html,const char *tag,int tagsize) { html->styleEngine->inheritBackgroundColor(); - tagIsDisplayEnabled = true; }
/* @@ -3023,7 +3000,6 @@
a_Html_tag_set_align_attr (html, &props, tag, tagsize); html->styleEngine->setNonCssHints (&props); - tagIsDisplayEnabled = true; }
/* @@ -3418,11 +3394,11 @@ ListItem *list_item; int *list_number; char buf[16]; - + /* Get our parent tag's variables (used as state storage) */ list_number = &html->stack->getRef(html->stack->size()-2)->list_number; ref_list_item = &html->stack->getRef(html->stack->size()-2)->ref_list_item; - + HT2TB(html)->addParbreak (0, wordStyle);
list_item = new ListItem ((ListItem*)*ref_list_item,prefs.limit_text_width); @@ -3430,7 +3406,7 @@ HT2TB(html)->addParbreak (0, wordStyle); *ref_list_item = list_item; S_TOP(html)->textblock = html->dw = list_item; - + if (style->listStyleType == LIST_STYLE_TYPE_NONE) { // none } else if (style->listStyleType >= LIST_STYLE_TYPE_DECIMAL) { @@ -3496,15 +3472,11 @@ /* Parse attributes that can appear on any tag */ Html_parse_common_attrs(html, tag, tagsize);
- /* Html_tag_open* function can set this to true to enable - * generic CSS display handling. - */ - tagIsDisplayEnabled = false; - /* Call the open function for this tag */ - _MSG("Open : %s\n", Tags[ni].name); - Tags[ni].open (html, tag, tagsize); - - if (tagIsDisplayEnabled) { + if (! html->styleEngine->initialized () + || html->styleEngine->style ()->display == DISPLAY_NONE) { + /* Call the open function for this tag */ + Tags[ni].open (html, tag, tagsize); + switch (html->styleEngine->style ()->display) { case DISPLAY_BLOCK: Html_display_block(html); @@ -3549,7 +3521,6 @@ (strchr(" \"'", tag[tagsize-3]) || /* [ "']/> */ (size_t)tagsize == strlen(Tags[ni].name) + 3))) { /* <x/> */
- _MSG("Close: %s\n", Tags[ni].name); Html_tag_cleanup_at_close(html, ni); /* This was a close tag */ html->ReqTagClose = false; diff -r 440fa7e952bd src/styleengine.cc --- a/src/styleengine.cc Tue Dec 08 18:46:31 2009 +0100 +++ b/src/styleengine.cc Wed Dec 09 00:48:03 2009 +0100 @@ -567,6 +567,10 @@ return Style::create (layout, &attrs); }
+bool StyleEngine::initialized () { + return stack->getRef (stack->size () - 1)->style != NULL; +} + /** * \brief Create a new style object based on the previously opened / closed * HTML elements and the nonCssProperties that have been set. @@ -585,7 +589,7 @@ // If this assertion is hit, you need to rearrange the code that is // doing styleEngine calls to call setNonCssHints() before calling // style() or wordStyle() for each new element. - assert (stack->getRef (stack->size () - 1)->style == NULL); + assert (! StyleEngine::initialized());
// reset values that are not inherited according to CSS attrs.resetValues (); diff -r 440fa7e952bd src/styleengine.hh --- a/src/styleengine.hh Tue Dec 08 18:46:31 2009 +0100 +++ b/src/styleengine.hh Wed Dec 09 00:48:03 2009 +0100 @@ -59,6 +59,7 @@ return NULL; };
+ bool initialized (); void parse (DilloHtml *html, DilloUrl *url, const char *buf, int buflen, CssOrigin origin); void startElement (int tag);