Closed Bug 58646 Opened 23 years ago Closed 22 years ago

[PATCH] broken images don't get inserted


(Core :: Layout, defect, P2)






(Reporter: sujay, Assigned: attinasi_layout)



(Whiteboard: [behavior]EDITORBASE+[adt2])


(3 files, 3 obsolete files)

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.
Depends on: alttext
assigning to cmanske, hopefully we can get this one in soon
Assignee: beppe → cmanske
Summary: broken images don't get inserted(composer tracking bug) → broken images don't get inserted
Target Milestone: --- → mozilla0.9
*** Bug 58782 has been marked as a duplicate of this bug. ***
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?
Keywords: meta
Summary: broken images don't get inserted → broken images don't get inserted [TRACKING]
Adding status to track important bugs that will be fixed automatically or with
minimum work once dependent bug is fixed.
Whiteboard: DEPENDS
moving to mozilla0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
Nothing we can do in editor code, so removing from 0.9.1 radar
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Keywords: correctness
Whiteboard: DEPENDS → [behavior]DEPENDS
moving to 1.0
Target Milestone: mozilla0.9.2 → mozilla1.0
No longer depends on: alttext
spam composer change
Component: Editor: Core → Editor: Composer
Whiteboard: [behavior]DEPENDS → [behavior]DEPENDS, EDITORBASE (0 days by Composer team)
With the changes to bug 102281, ou will see the image if you give it a width and
changing milestone
Target Milestone: mozilla1.0 → mozilla0.9.8
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.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Dependency bug is going to make nothing appear as well according to
Whiteboard: [behavior]DEPENDS, EDITORBASE (0 days by Composer team) → [behavior]DEPENDS, EDITORBASE+ (0 days by Composer team)
Keywords: nsbeta1+
changing milestone
Target Milestone: mozilla0.9.9 → mozilla1.0
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.
Assignee: cmanske → attinasi
Component: Editor: Composer → Layout
Keywords: meta
Summary: broken images don't get inserted [TRACKING] → broken images don't get inserted
Whiteboard: [behavior]DEPENDS, EDITORBASE+ (0 days by Composer team) → [behavior]DEPENDS, EDITORBASE+
sujay: please change QA contact
Whiteboard: [behavior]DEPENDS, EDITORBASE+ → [behavior]EDITORBASE+
>Contact mjudge to find out how to detect an editor when in frame code.

How about using CSS to specify 'broken image' behaviour?
Chris, I'm putting you down as the qa_contact...correct it if its wrong...
QA Contact: sujay → petersen
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
'', 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).

Summary: broken images don't get inserted → [PATCH] broken images don't get inserted
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.

Attached file Improved testcase
I simply put a number in front of each image to make it easier to see which
displayed and which didn't
Attachment #72113 - Attachment is obsolete: true
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.
How are they "valid URIs"? 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;
> How are they "valid URIs"? 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.
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.
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?
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...
removing myself from the cc list
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!

attinasi --> attinasi_layout to get on my priority radar (don't try this at
home, kids)
Assignee: attinasi → attinasi_layout
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?
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.
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
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.
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.
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.
Summary: [PATCH] broken images don't get inserted → [PARTIAL PATCH] broken images don't get inserted
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.
Attachment #73309 - Attachment is obsolete: true
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
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?
Priority: P3 → P2
Summary: [PARTIAL PATCH] broken images don't get inserted → [PATCH] broken images don't get inserted
adt2 per adt triage
Whiteboard: [behavior]EDITORBASE+ → [behavior]EDITORBASE+[adt2]
The patch applied and worked correctly on my Mac build (CARBON). I think it is
ready for prime-time review...
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
Attachment #75503 - Flags: review+
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 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
Attachment #75503 - Flags: approval+
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,
Fixes checked in. I'll look at what you mentioned David - thanks.
Closed: 22 years ago
Resolution: --- → FIXED
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.

Marking verified under Mac OS 9.1 (2002-03-28-08), OS X (2002-03-28-08) and
Windows ME (2002-03-28-03).
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.
You need to log in before you can comment on or make changes to this bug.