Last Comment Bug 607267 - Image maps are display:block, but the HTML spec says they should be inline
: Image maps are display:block, but the HTML spec says they should be inline
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: quirks-mode-spec
  Show dependency treegraph
 
Reported: 2010-10-26 04:05 PDT by Jesper Kristensen
Modified: 2012-08-14 05:20 PDT (History)
9 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't style imagemaps as block. (2.69 KB, patch)
2012-07-06 12:35 PDT, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review
testcase (from patch) (50 bytes, text/html)
2012-07-06 14:39 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
a11y test fixes to deal with this change (1.74 KB, patch)
2012-07-08 11:29 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Don't style imagemaps as block. (4.24 KB, patch)
2012-07-15 21:13 PDT, Boris Zbarsky [:bz]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Jesper Kristensen 2010-10-26 04:05:33 PDT
I came across a website today, which used image maps like so: <img usemap=#m1 ...><map name=m1>...</map><img usemap=#m2 ...><map name=m2>...</map>. The site expected the two images to be displayed on the same line, but in Firefox they were displayed on top of each other. In Internet Explorer 9 in all four document modes, the two images are displayed on one line.

In http://mxr.mozilla.org/mozilla-central/source/layout/style/html.css#129 Firefox declares the <map> element as a block level element. Bug 57054 made <map> inline for quirks mode documents, but from what I can read in the standards, <map> should be inline in standards mode as well.

From HTML 4:
http://www.w3.org/TR/html4/sgml/dtd.html#inline
http://www.w3.org/TR/html4/sgml/dtd.html#special

From HTML 5:
http://www.w3.org/TR/html5/rendering.html#display-types
Comment 1 Boris Zbarsky [:bz] 2010-11-05 13:51:47 PDT
David, any objections to just making this change?
Comment 2 j.j. 2012-07-02 18:56:26 PDT
No other browser has it (WebKit, Opera, IE8).
Was there ever a standard who required this? It's not in CSS 2.1 Appendix D.

And it would save one rule in quirks.css:

  map { display: inline }
Comment 3 Boris Zbarsky [:bz] 2012-07-06 12:29:46 PDT
> Was there ever a standard who required this?

HTML4 allows <map> to contain %block; which is a bit silly if it's not a block itself.

That said, I think we should just make this change.
Comment 4 Boris Zbarsky [:bz] 2012-07-06 12:35:39 PDT
Created attachment 639759 [details] [diff] [review]
Don't style imagemaps as block.
Comment 5 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-07-06 14:39:19 PDT
Created attachment 639803 [details]
testcase (from patch)
Comment 6 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-07-06 14:41:21 PDT
Comment on attachment 639759 [details] [diff] [review]
Don't style imagemaps as block.

r=dbaron
Comment 7 Boris Zbarsky [:bz] 2012-07-08 11:29:04 PDT
Created attachment 640082 [details] [diff] [review]
a11y test fixes to deal with this change
Comment 8 Trevor Saunders (:tbsaunde) 2012-07-09 18:52:39 PDT
Comment on attachment 640082 [details] [diff] [review]
a11y test fixes to deal with this change


>+      // map group.  Imagemaps are inlines by default, so TEXT_CONTAINER.
>       accTree =
>-        { PARAGRAPH: [
>+        { TEXT_CONTAINER: [

this part looks correct.

>+++ b/accessible/tests/mochitest/treeupdate/test_imagemap.html
>@@ -327,17 +327,19 @@
>       }
> 
>       this.finalCheck = function insertMap_finalCheck()
>       {
>         var accTree =
>           { SECTION: [
>             { IMAGE_MAP: [
>               { LINK: [ ] }
>-            ] }
>+            ] },
>+            // And a TEXT_LEAF for the imagemap itself
>+            { TEXT_LEAF: [] }

this seems wrong.  The comment sort of explains it, the accessible for the imagemap should be the object with role IMAGE_MAP and it shouldn't have a second accessible that's a text leaf.  I'm not sure exactly which case of creating a accessible by frame type is triggering this in nsAccessibilityService.cpp.  The solution is probably to add some sort of special case there or in nsTextFrame::CreateAccessible() does it make sense that the frame for an image map is now a nsTextFrame?

sorry about the delay in looking at this.
Comment 9 Boris Zbarsky [:bz] 2012-07-09 22:31:33 PDT
> the accessible for the imagemap should be the object with role IMAGE_MAP

That's the accessible for the _image_.

> I'm not sure exactly which case of creating a accessible by frame type is triggering this
> in nsAccessibilityService.cpp.

I could step through it, I guess.  I'll take a look and comment with the results.

> does it make sense that the frame for an image map is now a nsTextFrame?

It's not.  It's an nsInlineFrame, but probably contains an nsTextFrame.
Comment 10 Trevor Saunders (:tbsaunde) 2012-07-10 04:06:58 PDT
(In reply to Boris Zbarsky (:bz) from comment #9)
> > the accessible for the imagemap should be the object with role IMAGE_MAP
> 
> That's the accessible for the _image_.

true, but perhaps I should be more precise.  I think the idea here was that in the accessible tree the image map and the image should be one object.  So the tree would look like

{ <div containing image map> {
  { accessible for image (is image map instead of normal image accessible object since there'll be kids) [
    <accessibles for areas>
  ]}
]}

sorry about the js diagram, I need to learn to ascii art this sort of thing.

The idea being people who look at the accessible tree don't need to think about the image and map being different things.

> > I'm not sure exactly which case of creating a accessible by frame type is triggering this
> > in nsAccessibilityService.cpp.
> 
> I could step through it, I guess.  I'll take a look and comment with the
> results.

ok, thanks!

> > does it make sense that the frame for an image map is now a nsTextFrame?
> 
> It's not.  It's an nsInlineFrame, but probably contains an nsTextFrame.

ok, I'd say certanly since nsTextFrames are the only thing that get TextLeafAccessibles (the only overload of nsIFrame::CreateAccessible() to call nsAccessibilityService::CreateHTMLTextLeafAccessible())
Comment 11 Boris Zbarsky [:bz] 2012-07-14 21:23:52 PDT
> I could step through it, I guess.  I'll take a look and comment with the results.

OK, so the story is that the test has this markup:

  <div id="container">
    <img id="imgmap" width="447" height="15"
         usemap="#atoz_map"
         src="../letters.gif">
  </div>

and then it creates a <map> element and does:

  this.containerNode.appendChild(map);

where this.containerNode is the <div id="container"> and "map" is the <map>.  The resulting frametree, with this patch, looks like this:

  Block(div)(10)@0x12c7f2670 [content=0x11fee7500] {0,3732,55440,1236} [state=0000120000100010] sc=0x12c7708a0(i=1,b=0)<
    line 0x12c7f39b0: count=3 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,26820,1236} <
      ImageFrame(img)(1)@0x12c7f3908 {0,0,26820,900} [state=000000c000300010] [content=0x12f5f2900] [sc=0x12c7f3868] [src=chrome://mochitests/content/a11y/accessible/letters.gif]
      Text(2)"\n  "@0x12d208030 [run=0x127b33080][0,3,T]  next=0x12c7f2518 {26820,180,0,960} [state=00000000c8400010] [content=0x11fee7680] sc=0x127cf6e08 pst=:-moz-non-element
      Inline(map)(3)@0x12c7f2518 {26820,180,0,960} [state=0000100000000000] [content=0x12e40bb30] [sc=0x12c76f9e8]<>
    >
  >

and the TEXT_LEAF is the accessible for that nsTextFrame containing the text "\n  ".  When the <map> used to be a block, frame construction optimized that textframe away, because it was on a block boundary, but now it gets created....

Trevor, thoughts?
Comment 12 Boris Zbarsky [:bz] 2012-07-15 21:13:34 PDT
Created attachment 642477 [details] [diff] [review]
Don't style imagemaps as block.
Comment 13 alexander :surkov 2012-07-15 23:23:02 PDT
(In reply to Boris Zbarsky (:bz) from comment #11)

> and the TEXT_LEAF is the accessible for that nsTextFrame containing the text
> "\n  ".  When the <map> used to be a block, frame construction optimized
> that textframe away, because it was on a block boundary, but now it gets
> created....

if it's visible then there's no problem, if it's not then it should be fixed. Comment in the patch says "random textframes", it confuses.
Comment 14 Boris Zbarsky [:bz] 2012-07-16 06:42:03 PDT
Whether it's visible or not is a non-local decision: it depends on the styles of the <map>, on what comes after the <map>, and so forth.

Suggestions for what the comment should say are welcome!
Comment 15 Boris Zbarsky [:bz] 2012-07-17 09:22:57 PDT
I took out the "random" bit and clarified why we want to avoid the textframes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf567772a2f7
Comment 16 Ed Morley [:emorley] 2012-07-18 05:56:42 PDT
https://hg.mozilla.org/mozilla-central/rev/cf567772a2f7
Comment 17 alexander :surkov 2012-07-20 18:17:45 PDT
(In reply to Boris Zbarsky (:bz) from comment #14)
> Whether it's visible or not is a non-local decision: it depends on the
> styles of the <map>, on what comes after the <map>, and so forth.

I see

(In reply to Boris Zbarsky (:bz) from comment #15)
> I took out the "random" bit and clarified why we want to avoid the
> textframes.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cf567772a2f7

thank you

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