58646 - [PATCH] broken images don't get inserted

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[sujay]

using 10/30 build of netscape

1) launch netscape
2) launch composer
3) insert a bogus image(foobar.gif)

notice you don't see a broken image icon in the document.

Kathy told me to file this as a tracking bug. 

Simon did file a bug (40623) which got marked as a DUP of a layout
bug (41924), but we would like to keep this tracking bug open
in case there are additional composer-specific issues in addition
to 41924

all platforms.

Comment 1

[rubydoo123]

assigning to cmanske, hopefully we can get this one in soon

Comment 2

[Charles Manske]

*** Bug 58782 has been marked as a duplicate of this bug. ***

Comment 3

[Kathleen :Brade]

It's not clear to me that #58782 should be a duplicate of this bug since this is 
a tracking bug and that is a very specific bug.  Perhaps it should be duplicated 
to #41924?

Comment 4

[Charles Manske]

Adding status to track important bugs that will be fixed automatically or with
minimum work once dependent bug is fixed.

Comment 5

[rubydoo123]

moving to mozilla0.9.1

Comment 6

[Charles Manske]

Nothing we can do in editor code, so removing from 0.9.1 radar

Comment 7

[rubydoo123]

moving to 1.0

Comment 8

[Syd Logan]

spam composer change

Comment 9

[Marc Attinasi]

With the changes to bug 102281, ou will see the image if you give it a width and
height...

Comment 10

[Charles Manske]

changing milestone

Comment 11

[Charles Manske]

Marc: That is fine for images we add or change properties of in Composer, as 
we always write out the width and height, but there will still be images in 
existing pages that don't have those attributes, so this is still a problem.

Comment 12

[Syd Logan]

Dependency bug is going to make nothing appear as well according to
[e-mail]

Comment 13

[Charles Manske]

changing milestone

Comment 14

[Charles Manske]

I discussed this with Marc and he agrees it should be fixed in layout.
To avoid endless discussions with how broken images should work in the browser,
fixing this so image only appears in Composer window is totally acceptable.
Contact mjudge to find out how to detect an editor when in frame code.

Comment 15

[Charles Manske]

sujay: please change QA contact

Comment 16

[Simon Fraser [no longer active]]

>Contact mjudge to find out how to detect an editor when in frame code.

How about using CSS to specify 'broken image' behaviour?

Comment 17

[sujay]

Chris, I'm putting you down as the qa_contact...correct it if its wrong...

Comment 18

[Marc Attinasi]

I have a patch for this.  Basically, if the image specified is invalid, as in
'foo.jpg', then we assign a minimal size for the image (suitable for display of
the ICON) and it shows up in composer (yeah!).  If the URL is valid, like
'http://www.foo.org/foo.jpg', but the image is unavailable, aka broken, then we
do the usual behavior of showing the alt text inline.

The common case for composer, where you insert image, type a bogus name like
'foo.jpg' and press OK causes the icon to appear.  I think this is what we want,
and it does not impact the old behavior of inline alt text expansion vs. sized
box layout etc (the sticky and nasty religious battle rages on, we do not want
to go there to fix this).

Comment 19

[Marc Attinasi]

Attachment: testcase showing various broken image scenarios

Comment 20

[Marc Attinasi]

Attachment: PATCH: please check it out, see if it meets Composer's needs

Comment 21

[Charles Manske]

Definite progress! Thanks for the prompt service!
After applying patch, test cases 1, 2, 4, 5, and 7 display perfectly.
For cases 3 and 6, I don't see any "broken" image.
For case 6, the inline "alt" text is really bad for composer! Even worse than no
image at all, since user can alter the inline alt text. What is weird is that
looking in HTML source after typing inside that alt text shows nothing! Is this
anonymous content?
So 3 and 6 are obviously related in that they have a bogus http address.

Comment 22

[Charles Manske]

Attachment: Improved testcase

I simply put a number in front of each image to make it easier to see which
ones
displayed and which didn't

Comment 23

[Marc Attinasi]

cases 3 and 6 may have to get special treatment for composer. They represent
valid URIs but broken images.  Since they have no size, we do not create the box
for them.  In 'composer mode' we might want to force the box at some minimum
width, like we do for the invalid URI cases.  We don't want to do this for
non-composer mode, I think.

Comment 24

[Charles Manske]

How are they "valid URIs"? www.foo.org doesn't exist? 
If you want to test for editor, 
  PRInt16 displaySelection
  result = shell->GetSelectionFlags(&displaySelection);
where shell = presshell
PRBool isEditor = displaySelection == nsISelectionDisplay::DISPLAY_ALL;

Comment 25

[Marc Attinasi]

> How are they "valid URIs"? www.foo.org doesn't exist? 
They are valid in that the nsURI calss does not have a fit when they are
provided as input :)  What I saw was that in the cases where the URL class
asserted and returned errors, we were blowing it in the nsImageFrame class, so
that is what I patched.  For the case where the URI is OK, but the image is not
loaded because it is not accessable, we use our normal logic which relies on the
width / height to switch to the box-with-icon rendering.  I'll update that to
make considerations for composer, such that valid URL that points to a broken
image will result in a 'box' even if the width and height are not specified.

Comment 26

[Marc Attinasi]

Charley, the check for being in the editor works OK if I create a blank document
and edit it, however if I open a testcase in the browser, then choose 'Edit
Page' from the menu, that test comes back saying I am not in the editor.  Is
that intentional?

So, I have a patch to handle the case where an image with a well-formed but
inaccessible URL is used for an image and no size is specified.  In composer,
that image will result in a box with the broken image icon. In the browser, it
will turn into inline alt text (or, if no alt text is specified, a blank area).

I'd like to handle the Edit Page case though, so I'll wait to attach he patch.

Comment 27

[Charles Manske]

Marc: certainly not! that selection flag should apply when editing any page,
independent of how you created it. I wonder if it's a timinging issue? 
I.e., flag isn't set yet during document and image loading?
Mike: when are SelectionFlags set for editor? Can we move it up in time?

Comment 28

[Marc Attinasi]

Charley, it is a timing issue, sort of.  Basically, the document is loaded
BEFORE the editor is even instantiated. Here is the stack where the selection
state is actually set (in Init):

nsEditor::Init(nsEditor * const 0x04cd5100, nsIDOMDocument * 0x051dc9b4,
nsIPresShell * 0x05205530, nsIContent * 0x00000000, nsISelectionController *
0x05205540, unsigned int 0) line 322
nsPlaintextEditor::Init(nsPlaintextEditor * const 0x04cd5100, nsIDOMDocument *
0x051dc9b4, nsIPresShell * 0x05205530, nsIContent * 0x00000000,
nsISelectionController * 0x05205540, unsigned int 0) line 230 + 29 bytes
nsHTMLEditor::Init(nsHTMLEditor * const 0x04cd5100, nsIDOMDocument * 0x051dc9b4,
nsIPresShell * 0x05205530, nsIContent * 0x00000000, nsISelectionController *
0x05205540, unsigned int 0) line 278 + 29 bytes
nsEditorShell::InstantiateEditor(nsIDOMDocument * 0x051dc9b4, nsIPresShell *
0x05205530) line 952 + 55 bytes
nsEditorShell::DoEditorMode(nsIDocShell * 0x04e0e0d0) line 1011 + 26 bytes
nsEditorShell::PrepareDocumentForEditing(nsIDOMWindow * 0x04e239a4, nsIURI *
0x051f7740) line 477 + 20 bytes
nsEditorShell::EndPageLoad(nsIDOMWindow * 0x04e239a4, nsIChannel * 0x051f79e8,
unsigned int 0) line 4828 + 27 bytes
nsEditorShell::OnStateChange(nsEditorShell * const 0x051f9c08, nsIWebProgress *
0x04e0e9e4, nsIRequest * 0x051f79e8, int 786448, unsigned int 0) line 4597
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x04e0e9e4, nsIRequest *
0x051f79e8, int 786448, unsigned int 0) line 1110
nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * 0x051f79e8, unsigned int 0)
line 750
nsDocLoaderImpl::DocLoaderIsEmpty() line 647
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x04e0e9d4, nsIRequest *
0x040490b8, nsISupports * 0x00000000, unsigned int 0) line 578

InstantiateEditor is called AFTER the document is loaded, but the images are
loaded during the document load.  Ack, it looks like the selection state has to
be set really really early, before the document is even loaded.  I think I
shoudl leave that to another bug :)  Please advise...

Comment 29

[rubydoo123]

removing myself from the cc list

Comment 30

[Marc Attinasi]

Attachment: New And Improved patch - handles case where image URL is wellformed, but image is missing, in composer

Comment 31

[Marc Attinasi]

With the patch I just attached, we still have a problem where loading an
existing page (like the testcase) into the editor will not cause some of the
unsized images to show up.  This is due to the presentation shell not having the
selection set to nsISelectionDisplay::DISPLAY_ALL until after the document is
loaded.  However, this problem needs to be fixed anyway, and the patch attached
covers the more common case of adding an image with an invalid URL from the UI,
where we really need the icon to show up :)

Please check it out so we can proceed to the trunk - and beyond!

Comment 32

[Marc Attinasi]

attinasi --> attinasi_layout to get on my priority radar (don't try this at
home, kids)

Comment 33

[Simon Fraser [no longer active]]

I still think that a better approach would be to implement some (moz internal?)
CSS attributes that we can use to control how broken images render. How hard
would it be to add a new CSS attribute which applies to broken images?

Comment 34

[Charles Manske]

The new patch works as well (or bad?) as the previous, but it obviously should
work better once we figure out how to set nsISelectionDisplay::DISPLAY_ALL
earlier so the editor is detected during image loading.
Results in the test page currently show the 'broken image' box and icon for
all cases but 3 and 6 because they don't have image size attributes set.

Comment 35

[Marc Attinasi]

Simon, I don't understand how an internal-only css property (and pseudo-class)
is any better than doing it in the code.  If composer's stylesheets are going to
say something like

IMG:-moz-broken { -moz-display: -moz-box-and-icon; }

then why not just code it that way in C++?  What is the advantage of adding more
style rules and more internal-only properties? I guess we could expose the
propietary property to the general web authoring masses, but do they really care
enough to use such a property? I think they would just fix their broken images
first.

Comment 36

[Simon Fraser [no longer active]]

The main reason I suggest using CSS is to avoid more skanky plcaes in layout
code where we sniff the selection type to decide whether we're in composer or
not. This is a hack, and should be avoided if at all possible.

A benefit of doing this via CSS is that users can control how gecko displays
broken images in the browser by editing their userContent.css, though,
admittedly, not many will do this (although broken image handling has been a
long-running source of contention).

Also, having this in CSS means that we don't care about editor initialization
order. Editor applies stylesheets to the content when it gets instantiated, and
if those stylesheets contain rules that cause broken image display to change,
then layout will re-display appropriately.

Comment 37

[Marc Attinasi]

I'm not particularly fond of the idea of resolving style on image when the
loader tells us the load failed. It is worse even, because sometimes we do not
even try to load the images because the URL has an invalid format. Also, it is
not clear what style we would apply to the image even if we had the style rule.
Do we apply a width and height? Do we apply a background image for the icon? To
we render the alt text as content?  I will admit that I do like the idea of
being able to define the whole presentation of the broken image via CSS, but I
don't see how it can be done while still preserving the expected (legacy)
behavior of broken images (and without changin scads of code a week before
Mozilla 1.0). It seems to me like we would be using CSS to communicate a
'signal' to the image frame that says 'we want editor broken image semantics'
rather than communicating normal visual formatting semantics, like style rules
usually do.

Consider that we do have a :broken-image pseudoclass. In composer, we might be
tempted to write a rule into editorcontent.css that looks like:

img:broken-image {
  width: 16px;
  height: 16px;
  background-image: url(resource:/broken-image.gif);
}

but then what if the author had already specified a width and height via
attributes on the image tag? We do not preserve the author's dimensions.

Something like this might work:

img:broken-image {
  -moz-show-icon: 1;
}

and then the imag frame can look at this and determine if the icon should be
shown _regardless_ of the presence of width and height, but that is really the
same as having a pref or a flag to communicate this between the editor and the
image frame, not really usning CSS to style.
The initialization issues are still nagging.

Comment 38

[Marc Attinasi]

The current patch is only partial: I'm adding a style property to control this
now, so we don't have to check for composer using the presShell selection mode hack.

The style property will just signal that we want to force the icon - nothing else.

Comment 39

[Marc Attinasi]

Attachment: Fix the image sizing problems when the URL is malformed, and use style property -moz-force-broken-image-icon to force display of the broken imag icon in editor

Large patch: has editor change (css file), and all of the style changes needed
to implement a new -moz property. ImageFrame is also updated to handle invalid
URL format, and to use the new style property ot force display of the broken
image icon.

Comment 40

[Marc Attinasi]

Attachment: DOC describing how a new style property is added to the system

Attaching this here since I wrote it for this bug, but this doc will be checked
into the layout/doc directory once it has been reviewed and is deemed good to
go.

Comment 41

[Marc Attinasi]

Patch is ready to review. 

I'm applying it on the Mac now to make sure it merges, but assuming it does, can
others please try it out?

Comment 42

[Syd Logan]

adt2 per adt triage

Comment 43

[Marc Attinasi]

The patch applied and worked correctly on my Mac build (CARBON). I think it is
ready for prime-time review...

Comment 44

[karnaze (gone)]

Comment on attachment 75503 [details] [diff] [review]
Fix the image sizing problems when the URL is malformed, and use style property -moz-force-broken-image-icon to force display of the broken imag icon in editor

r=karnaze. As we discussed, please make sure not to check in the change to
nsHTMLOptionElement.cpp

Comment 45

[Marc Attinasi]

Kin suggested that I cleanup the extra 'return NS_STYLE_HINT_VISUAL;' in the
CalcDifference method. I did that, and he sr'd the bug.

Comment 46

[Asa Dotzler [:asa]]

Comment on attachment 75503 [details] [diff] [review]
Fix the image sizing problems when the URL is malformed, and use style property -moz-force-broken-image-icon to force display of the broken imag icon in editor

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The nsCSSStyleRule.cpp and nsRuleNode.cpp changes are inconsistent with
surrounding code and thus the 'inherit' value won't work.  It's probably a more
serious problem if someone else copies that incorrect code for another, real,
property.

Comment 48

[Marc Attinasi]

Fixes checked in. I'll look at what you mentioned David - thanks.

Comment 49

[Marc Attinasi]

Oh, I see what you mean. Right, I never meant to support 'inherit' for that
property, do you think I should comment it as such to prevent confusion? Then
again, supporting 'inherit' would not be a big deal either, I just didn't think
it necessary.

Comment 50

[Chris Petersen]

Marking verified under Mac OS 9.1 (2002-03-28-08), OS X (2002-03-28-08) and
Windows ME (2002-03-28-03).

Comment 51

[Dawn Endico]

I'm removing this release note item for this bug from the Mozilla 1.0 and
future versions of the release notes because this bug is marked fixed.

Mail me if you think this item should be re-added.